Optimize MimePath#validate by first querying the MimePath cache before attempting to parse #3976
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I was annoyed by the performance of scanning of JS sources and I had the feeling, that the performance dropped after the changes from #3516. While I'm still not sure whether performance suffered, I noticed a strange thing in the profile (npss in the file):
A significant potion of the total time is spend in A totally unscientific test looks promising: Baseline Patched: There could be pathological cases (the ones where @dbalek what do you think? |
vieiro
reviewed
Apr 13, 2022
| @@ -131,7 +131,7 @@ public final class MimePath { | |||
| "video" //NOI18N | |||
| )); | |||
|
|
|||
| private static final Map<String,Reference<MimePath>> string2mimePath = new HashMap<String,Reference<MimePath>>(); | |||
| private static final Map<String,Reference<MimePath>> string2mimePath = new HashMap<>(); | |||
There was a problem hiding this comment.
Synchronization looks ok to me, but what about Collections.synchronizedMap and forgetting about those synchronized blocks?
There was a problem hiding this comment.
Valid point, I switched to ConcurrentHashMap which should be better than the suggested synchronizedMap (finer Locking). What do you think?
There was a problem hiding this comment.
@matthiasblaesing ConcurrentHashMap is more performant. I assume we won't be storying nulls as values though.
| @@ -278,6 +278,12 @@ public static boolean validate(CharSequence type, CharSequence subtype) { | |||
| * @since 1.7 | |||
| */ | |||
| public static boolean validate(CharSequence path) { | |||
| String pathString = path == null ? null : path.toString(); | |||
There was a problem hiding this comment.
Doesn't parseImpl() immediately blow up with null anyway? Could simplify and let it throw NPE here?
neilcsmith-net
approved these changes
Apr 14, 2022
vieiro
approved these changes
Apr 15, 2022
…e attempting to parse For the changes regarding the switch from a synchronized access to a HashMap to a ConcurrentHashMap: The existing synchronisation only protected the get and set operations on the map and not some secondary functionality (iteration for example). Using a ConcurrentHashMap should thus improve performance and not change the semantics.
matthiasblaesing
force-pushed
the
mime_parser_performance
branch
from
5831665
to
7775ab1
Compare
|
@vieiro thanks for the additional check and yes this should be safe. Tests are happy, so lets get this in. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.