Modify

Opened 2 years ago

Closed 23 months ago

Last modified 23 months ago

#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)

13956_v1.patch (9.4 KB) - added by anonymous 2 years ago.
13956_v2.patch (12.6 KB) - added by GerdP 2 years ago.
13956_v3.patch (15.0 KB) - added by GerdP 2 years ago.
13956_v4.patch (15.1 KB) - added by GerdP 2 years ago.
13956_v5.patch (14.4 KB) - added by GerdP 2 years ago.

Download all attachments as: .zip

Change History (25)

Changed 2 years ago by anonymous

Attachment: 13956_v1.patch added

comment:1 Changed 2 years ago by anonymous

Not sure if that improves much. TagChecker stores additional values in its version of the map, so one needs additional code. See attached patch.

comment:2 Changed 2 years ago by GerdP

Oops, anonymous again. Patch was from me.

Changed 2 years ago by GerdP

Attachment: 13956_v2.patch added

comment:3 Changed 2 years ago by GerdP

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 2 years ago by Don-vip

Thanks!

  • one small typo: additinalPresetsValueData without "o"
  • yes getters are welcome :)

Changed 2 years ago by GerdP

Attachment: 13956_v3.patch added

comment:5 Changed 2 years ago by GerdP

Summary: Possible memory optimization with presets cache[Patch] Possible memory optimization with presets cache

comment:6 Changed 2 years ago by michael2402

A note since you are optimizing for memory/performance:

  • getPresetKeys does not need to create a copy - a simple unmodifiableSet 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 2 years ago by GerdP

Attachment: 13956_v4.patch added

comment:7 Changed 2 years ago by GerdP

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 2 years ago by michael2402

We can do this later.

If there are no objections, I would like to apply the patch the way it is now.

comment:9 Changed 2 years ago by Don-vip

Milestone: 16.1116.12

Milestone renamed

comment:10 Changed 2 years ago by Don-vip

Milestone: 16.1217.01

comment:11 Changed 2 years ago by Don-vip

The patch does not apply anymore, could you please update it?

Changed 2 years ago by GerdP

Attachment: 13956_v5.patch added

comment:12 Changed 2 years ago by Don-vip

Milestone: 17.0117.02

comment:13 Changed 2 years ago by Don-vip

Milestone: 17.0217.03

comment:14 Changed 2 years ago by Don-vip

Milestone: 17.0317.04

comment:15 Changed 23 months ago by Don-vip

Milestone: 17.0417.05

comment:16 Changed 23 months ago by bastiK

Resolution: fixed
Status: newclosed

In 12042/josm:

applied #13956 - memory optimization with presets cache (patch by GerdP)

comment:17 Changed 23 months ago by bastiK

Patch was a bit outdated, hope it is okay like this.

comment:18 Changed 23 months ago by stoecker

Cc: kendzi added

This change breaks "JOSM Integration" - I find the output always hard to read, but it seems kendzi3d is broken?

comment:19 Changed 23 months ago by bastiK

Looks more like matsim plugin.

comment:20 Changed 23 months ago by bastiK

In 12077/josm:

see #13956 - make cachePresets public

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.