Closed Bug 1245965 Opened 4 years ago Closed 11 months ago

Remove the NewObjectCache

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox47 --- affected

People

(Reporter: terrence, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

The NewObjectCache is a simple mechanism that allows us to avoid hitting the initialShapeTable and ObjectGroupCompartment::NewTable for matching object creations. This is already done implicitly in the JITs via templateObjects, so this is only useful for pure-interpreter workloads. I think it is only hot in Sunspider and maybe not even there.

In Bug 1211795 comment 10, Boris found that it is adding several hundred microseconds to minor collections on OSX because of the memset to sweep it. It is time for us to re-measure its effectiveness and cull it if it is not carrying its weight.
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=a410c6fc3f79

10 files changed, 3 insertions(+), 408 deletions(-)

And that's a bit of an understatement, given that I haven't removed Runtime-inl.h outright yet.
This shows no performance change from m-i, so looks like this can move forward. \o/
Blocks: 1246186
10 files changed, 7 insertions(+), 424 deletions(-)
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8716329 - Flags: review?(jdemooij)
Comment on attachment 8716329 [details] [diff] [review]
remove_NewObjectCache-v0.diff

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

Great! r++++, would review again.

::: js/src/jsobj.cpp
@@ +683,2 @@
>  bool
>  js::NewObjectWithTaggedProtoIsCachable(ExclusiveContext* cxArg, Handle<TaggedProto> proto,

Nit: I think this one can also be removed.
Attachment #8716329 - Flags: review?(jdemooij) → review+
Oooh, good catch. I think I missed it because it wasn't static -- exported, but appears to not be used elsewhere. Maybe an artifact from when half of this was in jsobjinlines.h?

I'll push as soon as the try run starts showing up green. I just want to make sure that ripping out Runtime-inl.h didn't anger our build system.
Ugh. The cache-hit path was not checking for OOMAfterAllocations, so this changes the position of all of our triggered OOMs. This is going to require some OOM fixing before it can land. Will put patches here as I work through the issues, since it will all need to land at once.
Keywords: leave-open
Does this need to remain open?
(In reply to :Ms2ger from comment #13)
> Does this need to remain open?

Yeah, the existing landings are for bugs that removing the NOC exposed. I did not land the patch itself because there were some timeouts on try that I suspected were caused by removal of the cache. I've been meaning to retry that and see what it looks like now that Try is generally greener.
Terrence is this still on your radar? I also have to hack this code for bug 1295967.

Regarding comment 0, if we need to keep the cache, we could add an |uint64_t generation;| to the table and to each entry, and purge() can just increment the table's generation. Entries in this table are pretty big anyway, so the uint64_t won't make much of a difference.
Flags: needinfo?(terrence)
(In reply to Jan de Mooij [:jandem] from comment #20)
> Terrence is this still on your radar? I also have to hack this code for bug
> 1295967.

Distressingly, if we remove the NewObjectCache, most of the tests on TreeHerder appear to time out. I don't really think it's that plausible though and I'd like to investigate what's really going on to cause the red before I close the bug. It's pretty low on my list right now though, so feel free to steal it.

> Regarding comment 0, if we need to keep the cache, we could add an |uint64_t
> generation;| to the table and to each entry, and purge() can just increment
> the table's generation. Entries in this table are pretty big anyway, so the
> uint64_t won't make much of a difference.

That's a good idea!
Flags: needinfo?(terrence)
I'll try to rebase this patch and see if it still causes test failures.

The cache is at least 7 KB on 64-bit for each context. It's not huge, but combined with the measurable impact on all (minor) GCs it's worth removing I think.
Attached patch Rebased patchSplinter Review
Updated patch. There were some CGC timeouts on Try, I added these tests to the list of tests that are slow with CGC. These tests allocate many thousands of objects and this used to hit the cache, but with the CGC zeal mode we trigger a CGC for each of these allocations, so the tests become much slower.
Assignee: terrence → jdemooij
Attachment #8785253 - Flags: review?(terrence)
Comment on attachment 8785253 [details] [diff] [review]
Rebased patch

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

\o/
Attachment #8785253 - Flags: review?(terrence) → review+
FYI, the patch regressed some tests on AWFY, most of them from Kraken.
(In reply to Guilherme Lima from comment #26)
> FYI, the patch regressed some tests on AWFY, most of them from Kraken.

Yeah, slightly more than I expected (also some 'assorted' tests) so I backed this out for now:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5768dbae15

I'd be happy to look into this more and come up with a better strategy, but I don't have the time right now. I'm also torn because "Would we accept these 450 lines nowadays given the wins that aren't that big?". Probably not. Anyway, let's take a closer look at this later, I guess.
Not working on this now and it regressed perf.
Assignee: jdemooij → nobody
Status: ASSIGNED → NEW

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Flags: needinfo?(sdetar)

Yeah let's close this given the perf regressions we got when we tried this last time. Maybe we can reconsider this at some point.

Status: NEW → RESOLVED
Closed: 11 months ago
Flags: needinfo?(sdetar)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.