Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2587823004:
Increase allocation register capacity for desktop platforms (Closed)
DescriptionIncrease allocation register capacity for desktop platforms
This CL increases the capacity of allocation register so that long
running sessions on desktop can run with --enable-heap-profiling.
BUG=673009
Committed: https://crrev.com/8b85e3550a8af978c6a969b00cd1e8c1d4d7aaaa
Cr-Commit-Position: refs/heads/master@{#441516}
Patch Set 1 #Patch Set 2 : Fix bucket size and hashing. #
Messages
Total messages: 19 (11 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
+Primiano. I am not sure if this would affect the hashing scheme or make it slower? But, we really seem to use more than 1.5mil allocation cells frequently.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
primiano@chromium.org changed reviewers: + dskiba@chromium.org
On 2016/12/19 21:26:29, ssid wrote: > +Primiano. I am not sure if this would affect the hashing scheme or make it > slower? But, we really seem to use more than 1.5mil allocation cells frequently. LGTM Re: bumping kAllocationBuckets to 1<<19. Right now 5M / (1<<18) would be a load factor of 5 when the HT is full, which might not be that terrible. Maybe run some perf benchmark with --enable-heap-profiling on and see if 18 vs 19 makes any noticeable difference. doubling the buckets (18 -> 19) would cost ~2 MB if I did the math correctly, which is not the end of the world. Maybe ask dskiba. I trust that a combination of you will take the right decision here.
On 2016/12/21 11:29:39, Primiano - Oh Oh Oh till Jan 3 wrote: > On 2016/12/19 21:26:29, ssid wrote: > > +Primiano. I am not sure if this would affect the hashing scheme or make it > > slower? But, we really seem to use more than 1.5mil allocation cells > frequently. > > LGTM > Re: bumping kAllocationBuckets to 1<<19. > Right now 5M / (1<<18) would be a load factor of 5 when the HT is full, which > > might not be that terrible. Maybe run some perf benchmark with > --enable-heap-profiling on and see if 18 vs 19 makes any noticeable difference. > doubling the buckets (18 -> 19) would cost ~2 MB if I did the math correctly, > which is not the end of the world. > Maybe ask dskiba. I trust that a combination of you will take the right decision > here. dskiba@ is OOO. 5M / (1<<18) is load factor of 19, 1.5M / (1<<18) is load factor of 5. 5M / (1<<19) is 9 and 5M / 1<< 20 is 4. Doubling the buckets surely increases the performance by 7-8% on smoothness.top_25_sites. The memory increase is not very significant with the noise in benchmarks. The total resident size, peak resident size and private dirty overall increased by 1-2.5MB But I just found that AddressHasher constants must be updated if I update the bucket count. So I think I will keep the bucket count uniform for all platforms. I will update the CL after trying some hashing benchmarks.
I ran some benchmarks on Chrome and there were only noise when I changed different values. So, I ran a micro benchmark which inserts and remove real chrome addresses and 15 for shift seems to perform better. I did not change the value of |a| since there was no observable difference with bigger primes.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2587823004/#ps20001 (title: "Fix bucket size and hashing.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483568929840240,
"parent_rev": "897ba6ebefe4ecea11090f8b9cf530f335c8cd4c", "commit_rev":
"7f416a8d63a26040aeae9bc633fd0e8baa48d2c6"}
Message was sent while issue was closed.
Description was changed from ========== Increase allocation register capacity for desktop platforms This CL increases the capacity of allocation register so that long running sessions on desktop can run with --enable-heap-profiling. BUG=673009 ========== to ========== Increase allocation register capacity for desktop platforms This CL increases the capacity of allocation register so that long running sessions on desktop can run with --enable-heap-profiling. BUG=673009 Review-Url: https://codereview.chromium.org/2587823004 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Increase allocation register capacity for desktop platforms This CL increases the capacity of allocation register so that long running sessions on desktop can run with --enable-heap-profiling. BUG=673009 Review-Url: https://codereview.chromium.org/2587823004 ========== to ========== Increase allocation register capacity for desktop platforms This CL increases the capacity of allocation register so that long running sessions on desktop can run with --enable-heap-profiling. BUG=673009 Committed: https://crrev.com/8b85e3550a8af978c6a969b00cd1e8c1d4d7aaaa Cr-Commit-Position: refs/heads/master@{#441516} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/8b85e3550a8af978c6a969b00cd1e8c1d4d7aaaa Cr-Commit-Position: refs/heads/master@{#441516}
Message was sent while issue was closed.
On 2017/01/04 23:47:24, commit-bot: I haz the power wrote: > Patchset 2 (id:??) landed as > https://crrev.com/8b85e3550a8af978c6a969b00cd1e8c1d4d7aaaa<span><p style="margin-top:0;margin-bottom:0">No data found. If a valid commit was specified this means the commit is not yet in a release.</p></span> > Cr-Commit-Position: refs/heads/master@{#441516} thanks a lot for the experiment! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
