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

Thread contention on Converters.PROPERTY_EDITOR #254

Closed
raulcaj opened this issue Sep 2, 2019 · 8 comments
Closed

Thread contention on Converters.PROPERTY_EDITOR #254

raulcaj opened this issue Sep 2, 2019 · 8 comments

Comments

@raulcaj
Copy link
Contributor

raulcaj commented Sep 2, 2019

Hi!

There is a problem of thread contention on the PropertyEditorManager.findEditor. It loads classes from the ClassLoader switch is a thread blocking operation.

Is it possible to cache this method result so it wouldn't be called again or it would break this functionality somehow?

It would go something like this:

			@Override
			Object tryConvert(Method targetMethod, Class<?> targetType, String text) {
				if (!canUsePropertyEditors())
					return SKIP;
				PropertyEditor editor = PROPERTY_BY_CLASS.get(targetType);
				if (editor == null) {
					editor = PropertyEditorManager.findEditor(targetType);
					if (editor != null)
						PROPERTY_BY_CLASS.put(targetType, editor);
				}
				if (editor == null)
					return SKIP;
				try {
					editor.setAsText(text);
					return editor.getValue();
				} catch (Exception e) {
					throw unsupportedConversion(e, targetType, text);
				}
			}
@lviggiano
Copy link
Collaborator

@raulcaj The classloader does cache by itself, indeed. Try doing some benchmark to verify.

@lviggiano
Copy link
Collaborator

lviggiano commented Sep 2, 2019

how did you verify that there is thread contention? I've not looked yet into it, but I know for sure that the classloader does caching.

can you please elaborate your evidences about this?

@raulcaj
Copy link
Contributor Author

raulcaj commented Sep 2, 2019

@lviggiano report
I had done a report for the company I work for. I'm sharing it with you.

@raulcaj
Copy link
Contributor Author

raulcaj commented Sep 2, 2019

also, I'd like to add that the result of isClassAvailable method should be cached too.

@lviggiano
Copy link
Collaborator

lviggiano commented Sep 2, 2019

ok then, sounds fine. If you want to send me a pull request I'll merge that.

@raulcaj
Copy link
Contributor Author

raulcaj commented Sep 2, 2019

Great! I'll just finish running some more tests to see if I catch something else related to this and I'll create it.

@lviggiano
Copy link
Collaborator

Thanks for the report; the improvement looks impressive.
I'll try to roll out a new version asap.

It's long time I'm not doing active development on this project, I need to cleanup two things, then I hope it won't take much time to release a new version on maven repository.

Today afternoon I was fighting with maven+intellij since I was not using my laptop at home for development.

lviggiano pushed a commit that referenced this issue Sep 2, 2019
Solves the thread contention problem reported on Issue #254
@lviggiano
Copy link
Collaborator

solved by #255

lviggiano pushed a commit that referenced this issue Jun 6, 2020
The bug was introduced by commit 1acb314 which was supposed to resolve
a thread contention issue #254, which now I think, it was not an issue
since it was required to have some synchronization mechanism to avoid
problems like the one happened here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants