#13956 closed enhancement (fixed)
[Patch] Possible memory optimization with presets cache
Reported by: | Don-vip | Owned by: | team |
---|---|---|---|
Priority: | normal | Milestone: | 17.05 |
Component: | Core | Version: | |
Keywords: | performance memory | Cc: | bastiK, GerdP, kendzi |
Description
It seems we have two maps storing the same data twice, for about ~400Kb of memory each:
- TagChecker.presetsValueData:
/** The spell check preset values */ private static volatile MultiMap<String, String> presetsValueData;
- AutoCompletionManager.PRESET_TAG_CACHE:
/** * the same as tagCache but for the preset keys and values can be accessed directly */ protected static final MultiMap<String, String> PRESET_TAG_CACHE = new MultiMap<>();
This cache could be moved to TaggingPresets class and used everywhere in the application.
Attachments (5)
Change History (25)
Changed 7 years ago by
Attachment: | 13956_v1.patch added |
---|
comment:1 Changed 7 years ago by
Changed 7 years ago by
Attachment: | 13956_v2.patch added |
---|
comment:3 Changed 7 years ago by
v2 also moves PRESET_ROLE_CACHE
from AutoCompletionManager
to TaggingPresets
. This allows to simplify the code in
AutoCompletionManager
.
Probably I should use getters instead of public structures in TaggingPresets?
comment:4 Changed 7 years ago by
Thanks!
- one small typo: additinalPresetsValueData without "o"
- yes getters are welcome :)
Changed 7 years ago by
Attachment: | 13956_v3.patch added |
---|
comment:5 Changed 7 years ago by
Summary: | Possible memory optimization with presets cache → [Patch] Possible memory optimization with presets cache |
---|
comment:6 Changed 7 years ago by
A note since you are optimizing for memory/performance:
getPresetKeys
does not need to create a copy - a simpleunmodifiableSet
would be enough.
Other than that, I think moving the cache to TaggingPresets is a matter of taste - but it seems to make the code cleaner.
I personally would change TaggingPresetItem
, so that cachePresetItem
can use the visitor pattern. It would make it easier for plugins and for us to add custom item types.
Changed 7 years ago by
Attachment: | 13956_v4.patch added |
---|
comment:7 Changed 7 years ago by
Yes, I don't know why I didn't use unmodifiableSet
here. I also think that TaggingPresets.getTaggingPresets()
should use
unmodifiableCollection
instead of creating a copy.
I've implemented these changes with v4.
Reg. visitor pattern: Sorry, I am not used to think in this pattern. Maybe you can create a 2nd patch which would help me to learn it.
comment:8 Changed 7 years ago by
We can do this later.
If there are no objections, I would like to apply the patch the way it is now.
comment:10 Changed 7 years ago by
Milestone: | 16.12 → 17.01 |
---|
Changed 7 years ago by
Attachment: | 13956_v5.patch added |
---|
comment:12 Changed 7 years ago by
Milestone: | 17.01 → 17.02 |
---|
comment:13 Changed 7 years ago by
Milestone: | 17.02 → 17.03 |
---|
comment:14 Changed 6 years ago by
Milestone: | 17.03 → 17.04 |
---|
comment:15 Changed 6 years ago by
Milestone: | 17.04 → 17.05 |
---|
comment:18 Changed 6 years ago by
Cc: | kendzi added |
---|
This change breaks "JOSM Integration" - I find the output always hard to read, but it seems kendzi3d is broken?
Not sure if that improves much.
TagChecker
stores additional values in its version of the map, so one needs additional code. See attached patch.