Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReloadableResourceBundleMessageSource performance issue when using many resource files [SPR-5476] #10149

Closed
spring-issuemaster opened this issue Feb 12, 2009 · 8 comments

Comments

@spring-issuemaster
Copy link
Collaborator

commented Feb 12, 2009

Nikos Beis opened SPR-5476 and commented

When using many property files and a cacheSeconds value of bigger than 0 the lookup time increases rapidly. I saw a 15% performance boost in my application when putting all my properties in the same file. The problem is the nested for loops in resolveCode and resolveCodeWithoutArguments.


Issue Links:

  • #15133 ReloadableResourceBundleMessageSource locks properties hashmap and fails under load. ("duplicates")
  • #14948 Allow adding resources to ReloadableResourceBundleMessageSource

3 votes, 6 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 18, 2011

Vincent Jouenne commented

I am experiencing performance issues as well. Under load, many threads are waiting for the lock on the Map in the getProperties method. The global lock on the Map prevents other threads to execute despite the efficiency of the check.

Locking on a PropertyHolder instead of the Map would make it better but ultimately it would be beneficial to perform the refresh on a background thread and swap the Map reference once the refresh is done.


protected PropertiesHolder getProperties(String filename) {
synchronized (this.cachedProperties) {

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 30, 2012

Emmanuel Vasseur commented

I confirm the performance issue.
Synchronized blocks lock reads, in this way it is better to use ConcurrentHashMap.

Furthermore there is no need to use a locking behavior. Messages can be load during spring context loading, and reload can be scheduled with a spring (or other) scheduling task.

Sure there is a risk on multiple handler replacements, but it can be evict if there is only one Map and if all methods look into only one time (by using a local handler).
If messages are preload, cachedMergedProperties is the only Map to keep.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 11, 2012

Chris Beams commented

Emmanuel, would you be interested in contributing a fix here? It sounds like you're already pretty familiar with the relevant code. Check out the contributor guidelines; we'd appreciate it!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 27, 2013

Lari Hotari commented

Grails would be happy to use an optimized solution too: http://jira.grails.org/browse/GRAILS-10852

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 28, 2013

Lari Hotari commented

I have rewrited the caching logic of ReloadableResourceBundleMessageSource for Grails: grails/grails-core@7ee51f6...GRAILS-10852 . I can port this to Spring when the work is finished.

https://github.com/grails/grails-core/blob/GRAILS-10852/grails-core/src/main/groovy/org/codehaus/groovy/grails/context/support/ReloadableResourceBundleMessageSource.java

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2014

Jan-Hendrik Kuperus commented

We've run into the problem where a lot of threads are locked during the refresh of this MessageSource. Especially if the underlying filesystem is slow.

I've modified the ReloadableResourceBundleMessageSource to allow for background reloading of source files. This should only be used in environments where it is not a problem to serve "stale" data while the data is being reloaded. Would you guys prefer a way to switch this behaviour inside the ReloadableResourceBundleMessageSource? Or would you rather have a separate class with this behaviour like ConcurrentReloadableResourceBundleMessageSource?

I'll read up on the contributor guidelines and prepare a pull request.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2014

Lari Hotari commented

Grails 2.4 uses this modified version of RRBMS class:
https://github.com/grails/grails-core/blob/2.4.x/grails-core/src/main/groovy/org/codehaus/groovy/grails/context/support/ReloadableResourceBundleMessageSource.java
to get around the blocking issues. I'm on mobile now so I cannot check if it serves the stale version while updating, but that's supported in the simple CacheEntry class that's used within a ConcurrentHashMap.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 4, 2016

Juergen Hoeller commented

This looks a duplicate of #15133 at this point, as released in 4.1. I don't really see much we can do beyond that. Please give 4.1/4.2 a try and feel free to reopen this issue if any concerns remain.

Juergen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.