Closed Bug 1174319 Opened 4 years ago Closed 4 years ago

Remove the intermediate store buffer buffer

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This would have been incredibly useful for inlining store buffer puts in the jits, but it turns out that's not actually that much of a bottleneck. I think we're not going to do this in the short term, so we can always re-add it easily enough if needed later.
Comment on attachment 8621842 [details] [diff] [review]
remove_storebuffer_intermediate_buffer-v0.diff

Never mind, this appears to dramatically increase our icache miss rate.
Attachment #8621842 - Flags: review?(jcoppeard)
Comment on attachment 8621842 [details] [diff] [review]
remove_storebuffer_intermediate_buffer-v0.diff

I experimented with a few more variants and ended up retesting this a few more times. Turns out I just got two 3 sigma runs in a row in the wrong direction. After a few more runs, things actually come out even or lightly in favor of removing the intermediate buffer.
Attachment #8621842 - Flags: review?(jcoppeard)
Depends on: 1174850
Comment on attachment 8621842 [details] [diff] [review]
remove_storebuffer_intermediate_buffer-v0.diff

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

I'm kind of surprised by this, but if your measurements say it's OK then let's do it.
Attachment #8621842 - Flags: review?(jcoppeard) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=014e576eaa15

Notably M(oth) and SM(cgc) are passing now, which I think was a problem with my tip yesterday.
https://hg.mozilla.org/mozilla-central/rev/ea281f1bffcb
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
A regression/improvement was reported on AWFY:
- slave: Win 8 32-bit (i7-4700HQ, browser)
- mode: Ion

Regression(s)/Improvement(s):
- octane: -2.71% (regression)
- octane: Richards: -3.17% (regression)
- octane: zlib: -1.78% (regression)
- ss: tagcloud: 24.29% (regression)
- dromaeo: sunspider-string-validate-input: -5.84% (regression)
- dromaeo: sunspider-bitops-nsieve-bits: -1.26% (regression)
- dromaeo: sunspider-controlflow-recursive: -1.13% (regression)
- dromaeo: sunspider-3d-raytrace: -2.04% (regression)
- dromaeo: sunspider-bitops-bitwise-and: -1.93% (regression)
- dromaeo: sunspider-access-nbody: -2.36% (regression)
- dromaeo: sunspider-math-spectral-norm: -1.76% (regression)
- dromaeo: sunspider-crypto-md5: -1.79% (regression)

Recorded range:
- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=43e3ece52f55&tochange=4c3af36331e0

More details: http://arewefastyet.com/regressions/#/regression/1786178
I reproduced this between one before this patch and tip on zlib. Between one patch before and this however, I see no difference, so it's definitely not this patch. Perhaps the next, will try it next.

Before -- Score (version 9): 59760
After -- Score (version 9): 59951
I think maybe I reproduced a regression in my MQ, because I see no difference between before and my current tip.
Backed out for now -- I don't have time to track this down while paying attention to work-week presentations.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Having the intermediate buffer be a single element makes construction of temporary RelocatablePtr much cheaper and simplifies this code tremendously, so I think we'll stick with that.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.