Bug 172710 - REGRESSION(r216772): Dromaeo jslib-modify-jquery crashes
Summary: REGRESSION(r216772): Dromaeo jslib-modify-jquery crashes
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-30 06:10 PDT by Yusuke Suzuki
Modified: 2017-05-30 09:06 PDT (History)
7 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-05-30 06:10:28 PDT
I've just found that Dromaeo jslib-modify-jquery crashes with ToT.
And I checked that this does not crashes on STP 30.
Comment 1 Yusuke Suzuki 2017-05-30 06:59:00 PDT
Bisected by using WebKit Nightly Archive.
https://trac.webkit.org/log/webkit/?stop_rev=216759&rev=216778
Comment 2 Yusuke Suzuki 2017-05-30 07:01:44 PDT
Since the process is killed by SIGKILL, I guess this is caused by r216772.
Comment 3 Andreas Kling 2017-05-30 07:42:11 PDT
Oh that's pretty nasty. Do you know why that test grows so huge? Are we not GC'ing often/aggressively enough?

Tip: If you run Safari with the "resource usage overlay" enabled (from the internal Debug menu), you can see when the next scheduled eden and full collections are going to happen.

It sounds strange that this test should have to exceed 4GB footprint, no? I would think that the objects created for each iteration would be transient.
Comment 4 Andreas Kling 2017-05-30 07:48:45 PDT
Or hmm, looking at the test, IIUC it measures jQuery performance by creating this fragment:

<strong>some text</strong>

...and inserting as many of them as possible into the document in a certain time. This kind of test is indeed going to hit our memory limits now that they are down to 4GB, since computers (and WebKit!) are too damn fast. :|
Comment 5 Yusuke Suzuki 2017-05-30 07:55:01 PDT
(In reply to Andreas Kling from comment #4)
> Or hmm, looking at the test, IIUC it measures jQuery performance by creating
> this fragment:
> 
> <strong>some text</strong>
> 
> ...and inserting as many of them as possible into the document in a certain
> time. This kind of test is indeed going to hit our memory limits now that
> they are down to 4GB, since computers (and WebKit!) are too damn fast. :|

Yeah, I've just checked it with resource overlay and seen that bmalloc memory becomes huge while GC heap is still ~300MB.
Comment 6 Andreas Kling 2017-05-30 08:05:21 PDT
(In reply to Yusuke Suzuki from comment #5)
> (In reply to Andreas Kling from comment #4)
> > Or hmm, looking at the test, IIUC it measures jQuery performance by creating
> > this fragment:
> > 
> > <strong>some text</strong>
> > 
> > ...and inserting as many of them as possible into the document in a certain
> > time. This kind of test is indeed going to hit our memory limits now that
> > they are down to 4GB, since computers (and WebKit!) are too damn fast. :|
> 
> Yeah, I've just checked it with resource overlay and seen that bmalloc
> memory becomes huge while GC heap is still ~300MB.

Most of that bmalloc memory is very likely a bajillion DOM nodes (<strong> elements and Text nodes), I bet. They probably all have a JS DOM wrapper each, but even if we could GC those we'd only recover the wrapper memory, since the nodes are still physically inserted in the DOM.

I'm unsure what the right solution would be here. I mean, if we raise the limit, there will still come a day when we get so fast at creating and inserting DOM nodes, that we exceed the new limit :)

+CC Geoff and Michael for thoughts
Comment 7 Geoffrey Garen 2017-05-30 08:51:53 PDT
This stinks. Is Dromaeo still maintained? Maybe we can report this as a bug, and suggest a change to the test to insert a fixed number of nodes and time the action, as opposed to inserting a fixed amount of time and counting the nodes.
Comment 8 Yusuke Suzuki 2017-05-30 09:06:14 PDT
(In reply to Geoffrey Garen from comment #7)
> This stinks. Is Dromaeo still maintained? Maybe we can report this as a bug,
> and suggest a change to the test to insert a fixed number of nodes and time
> the action, as opposed to inserting a fixed amount of time and counting the
> nodes.

Related issue is found.
https://bugzilla.mozilla.org/show_bug.cgi?id=1279035