Opened 9 years ago
Closed 9 years ago
#12272 closed defect (fixed)
[Patch] Fix access to the AbstractPrimitive#keys
Reported by: | michael2402 | Owned by: | team |
---|---|---|---|
Priority: | normal | Milestone: | 16.01 |
Component: | Core | Version: | |
Keywords: | Cc: |
Description
The keys array is synchronized using RCU.
This patch enforced this pattern. I also rewrote the comment to make people google about RCU instead of explaining everything from scratch.
We need to declare the keys array volatile, see http://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.3.1.4
We can skip the array duplication on key changes if we rewrite the keys reference after the change or if we use an AtomicReferenceArray.
Attachments (1)
Change History (5)
follow-up: 2 comment:1 by , 9 years ago
comment:2 by , 9 years ago
Replying to simon04:
Replying to michael2402:
I also rewrote the comment to make people google about RCU instead of explaining everything from scratch.
A link to a page explaining RCU would be nice in the Javadoc. :)
Good Idea. I'll search one.
We need to declare the keys array volatile, see http://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.3.1.4
How is performance affected by the added
volatile
keyword, e.g., when reading a huge OSM file?
Performance should not differ much - volatile simply enforces the memory to be really written once. So for the cases where the compiler behaved like it was intended by the old code there should not be any difference. It simply prevents the compiler from doing some optimisations that would break our synchronisation (e.g. move the newKeys[keyIndex] = key after the keys = newKeys line)
by , 9 years ago
Attachment: | patch-fix-keys-synchronisation.patch added |
---|
comment:3 by , 9 years ago
Milestone: | → 16.01 |
---|
Concerning performance: I verified this by measuring the time to read a country extract using OsmReader
– no notable difference.
And I'll fix (the|my) implementation of OsmPrimitive#hasSameInterestingTags
;)
Replying to michael2402:
A link to a page explaining RCU would be nice in the Javadoc. :)
How is performance affected by the added
volatile
keyword, e.g., when reading a huge OSM file?Checkstyle enforces line lenghts ⩽145 (source:trunk/tools/checkstyle/josm_checks.xml), would you please adapt your patch also in this respect.