Closed Bug 1424934 Opened 7 years ago Closed 4 years ago

Investigate using one byte per cell for marking information

Categories

(Core :: JavaScript: GC, enhancement, P5)

Product:
Core
Component:
JavaScript: GC
Type:
enhancement

Tracking

()

Status:
RESOLVED WONTFIX
Milestone:
mozilla74
Tracking Flags:
Tracking Status
firefox74 --- affected

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

backout-1424934
(deleted), patch
Details | Diff | Splinter Review
Currently we store mark bits in a per-chunk sparse bitmap with two bits allocated for every possible start position. To support parallel marking we'd like to move to having a single byte per cell. Probably this would be stored in the arena rather than at the end of the chunk. We could store them after the arena header before the first cell, for example. The current arrangement was chosen because it has good cache behaviour, so there may be perf implications to moving to a different arrangement. We should try this and see what difference it makes. It may be worth taking a small hit in throughput if it unlocks larger gains later on.
I just realised that this will complicate the implementation of atom marking: this copies the mark bits for arenas containing atoms and uses them to maintain per-zone bitmaps which represent an over-approximation of the atoms held live by that zone. This is done using bitwise operations, memcpy, etc. Using one byte per atom could increase the memory usage substantially. We may have to map atom marking information to a one-bit-per-atom bitmap to avoid this penalty.
Priority: -- → P3
It's possible that moving mark bits away from the allocations would be a security win (reducing ease of leveraging bounds-overwrites: see https://bugs.chromium.org/p/chromium/issues/detail?id=633030)
Blocks: ConcurrentMarking
Depends on: 1524424
Assignee: nobody → jcoppeard

Try is looking good and performance results don't show any major regressions. There's still room for improvement though.

Attachment #9117287 - Attachment description: Bug 1424934 - WIP: Investigate using one byte per cell for marking information → Bug 1424934 - Investigate using one byte per cell for marking information r?sfink
Attachment #9117287 - Attachment description: Bug 1424934 - Investigate using one byte per cell for marking information r?sfink → Bug 1424934 - Replace the chunk's mark bitmap with one byte per cell in the arena r?sfink
Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d389e3aa4522 Replace the chunk's mark bitmap with one byte per cell in the arena r=sfink,jandem

https://hg.mozilla.org/mozilla-central/rev/d389e3aa4522

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox74: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
Regressions: 1608423

Telemetry for 74 is now working and shows no discernable change from this so far.

Sadly it seems this is has caused a performance regression on android. Mark time telemetry is showing a sizeable increase on aarch64 for version 74 (this change landed two days after branching).

Mark time telemetry on aarch64:

                73       74, to 15th
5th Percentile  10.29    10.4  
25th Percentile 55.07    51.62 
Median          89.11    99.39 
75th Percentile 151.8    215.07
95th Percentile 492.97   712.5

Total GC time:

                73       74    
5th Percentile  25.6     25.9  
25th Percentile 93.98    87.69 
Median          148.45   165.7     +10%
75th Percentile 240.1    317.62    +32%
95th Percentile 723.69   965.18    +33%

Testing locally with a benchmark that constantly allocates shows a 10% time increase for long running GCs with this change.

I think this approach has been proved to work and can still be a stepping stone to further improvements (like concurrent marking) but until we have these other improvements ready to land we should back it out.

Backout by malexandru@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5fda66dee91f Backed out changeset d389e3aa4522 as requested by jonco:
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Priority: P3 → P5
Attachment #9117287 - Attachment is obsolete: true
Regressions: 1608097

== Change summary for alert #24772 (as of Thu, 23 Jan 2020 06:11:02 GMT) ==

Improvements:

1% Base Content JS windows7-32 opt 2,944,152.67 -> 2,929,577.33
1% Base Content JS windows7-32-shippable opt 2,944,152.67 -> 2,929,577.33
0.43% Base Content JS windows7-32-shippable opt 2,944,155.33 -> 2,931,488.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=24772

We're not planning to do this at the current time.

Status: REOPENED → RESOLVED
Closed: 5 years ago → 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: