Closed Bug 1506730 Opened 11 months ago Closed 11 months ago

remove PLDHashTable::Iterator::mStart

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(1 file)

We only use its value in one place, and said value is easily computable
from readily available information.  This change makes iterators
slightly smaller.
Comment on attachment 9024598 [details] [diff] [review]
remove PLDHashTable::Iterator::mStart

Review of attachment 9024598 [details] [diff] [review]:
-----------------------------------------------------------------

Iterators are always allocated on the stack, so making them smaller isn't much of a win.

Have you measured the benchmarks in xpcom/rust/gtest/bench-collections/Bench.cpp? When I was working on them I found that the iteration benchmark was highly sensitive to very minor changes in the code.
Attachment #9024598 - Flags: review?(n.nethercote)
I don't really understand the results, but apparently removing mStart makes a small (positive) difference.

Without patch:

[ RUN      ] BenchCollections.PLDHash

  succ_lookups  fail_lookups insert_remove       iterate         total
       96.8 ms       75.6 ms       26.2 ms       42.6 ms      241.3 ms
       52.3 ms       55.9 ms       22.0 ms       39.8 ms      170.0 ms
       52.4 ms       56.0 ms       21.8 ms       39.7 ms      170.0 ms
       52.3 ms       56.2 ms       22.1 ms       40.6 ms      171.2 ms
       51.8 ms       56.3 ms       22.0 ms       39.8 ms      170.0 ms

With patch:

[ RUN      ] BenchCollections.PLDHash

  succ_lookups  fail_lookups insert_remove       iterate         total
       73.2 ms       65.2 ms       23.7 ms       40.4 ms      202.5 ms
       51.1 ms       55.5 ms       21.8 ms       40.2 ms      168.6 ms
       51.2 ms       56.2 ms       22.0 ms       40.1 ms      169.4 ms
       51.2 ms       55.3 ms       21.9 ms       38.3 ms      166.7 ms
       48.6 ms       52.2 ms       20.0 ms       38.1 ms      158.9 ms

These results make zero sense to me; it's possible I'm not running them on a quiet enough machine or something?  OTOH, I can run the benchmark repeatedly with the same state and get differences of 10%, so I think the results fall in the noise.
Flags: needinfo?(n.nethercote)
Comment on attachment 9024598 [details] [diff] [review]
remove PLDHashTable::Iterator::mStart

Review of attachment 9024598 [details] [diff] [review]:
-----------------------------------------------------------------

The results are noisy in general, but the "iterate" result didn't change, so I'm happy with this.
Attachment #9024598 - Flags: review+
Flags: needinfo?(n.nethercote)
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/640b09ec036e
remove PLDHashTable::Iterator::mStart; r=njn
https://hg.mozilla.org/mozilla-central/rev/640b09ec036e
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.