Bug 151242 - [JSC] JSPropertyNameEnumerator could be destructorless.
Summary: [JSC] JSPropertyNameEnumerator could be destructorless.
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Performance
Depends on: 151285 151593
Blocks:
  Show dependency treegraph
 
Reported: 2015-11-12 19:07 PST by Andreas Kling
Modified: 2015-11-24 13:35 PST (History)
7 users (show)

See Also:


Attachments
Hack for EWS (6.58 KB, patch)
2015-11-12 19:08 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (7.30 KB, patch)
2015-11-12 19:22 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Follow-up patch to fix 32-bit platforms (4.26 KB, patch)
2015-11-13 11:23 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch for relanding (7.92 KB, patch)
2015-11-17 13:13 PST, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2015-11-12 19:07:05 PST
I think we could make JSPropertyNameEnumerator destructorless by allocating the m_propertyNames vector out of GC copied space instead.

The "vector" never changes size after construction anyway.
Comment 1 Andreas Kling 2015-11-12 19:08:38 PST
Created attachment 265463 [details]
Hack for EWS
Comment 2 Andreas Kling 2015-11-12 19:22:00 PST
Created attachment 265464 [details]
Patch

With missing CopyToken + ChangeLog.
Comment 3 Mark Lam 2015-11-12 21:53:36 PST
Comment on attachment 265464 [details]
Patch

r=me
Comment 4 WebKit Commit Bot 2015-11-13 07:59:28 PST
Comment on attachment 265464 [details]
Patch

Clearing flags on attachment: 265464

Committed r192416: <http://trac.webkit.org/changeset/192416>
Comment 5 WebKit Commit Bot 2015-11-13 07:59:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Csaba Osztrogonác 2015-11-13 10:03:19 PST
(In reply to comment #4)
> Comment on attachment 265464 [details]
> Patch
> 
> Clearing flags on attachment: 265464
> 
> Committed r192416: <http://trac.webkit.org/changeset/192416>

It made API tests assert on the 32 bit Apple Mac bots:
https://build.webkit.org/builders/Apple%20El%20Capitan%2032-bit%20JSC%20%28BuildAndTest%29
Comment 7 Csaba Osztrogonác 2015-11-13 10:05:57 PST
(In reply to comment #6)
> (In reply to comment #4)
> > Comment on attachment 265464 [details]
> > Patch
> > 
> > Clearing flags on attachment: 265464
> > 
> > Committed r192416: <http://trac.webkit.org/changeset/192416>
> 
> It made API tests assert on the 32 bit Apple Mac bots:
> https://build.webkit.org/builders/Apple%20El%20Capitan%2032-
> bit%20JSC%20%28BuildAndTest%29

and ~40 JSC tests fail on Apple Windows and 32 bit ARM bots:
- https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/55097
- https://build.webkit.org/builders/EFL%20Linux%20ARMv7%20Thumb2%20Release
- https://build.webkit.org/builders/EFL%20Linux%20ARMv7%20Traditional%20Release
Comment 8 Andreas Kling 2015-11-13 10:45:51 PST
(In reply to comment #6)
> (In reply to comment #4)
> > Comment on attachment 265464 [details]
> > Patch
> > 
> > Clearing flags on attachment: 265464
> > 
> > Committed r192416: <http://trac.webkit.org/changeset/192416>
> 
> It made API tests assert on the 32 bit Apple Mac bots:
> https://build.webkit.org/builders/Apple%20El%20Capitan%2032-
> bit%20JSC%20%28BuildAndTest%29

Aha! It looks like copied space allocations are expected to be 8-byte divisible on 32-bit platforms too, even though pointers are 4 bytes there. I will make a quick fix that rounds up allocation sizes to the nearest multiple of 8 (same idea as DirectArguments when it allocates out of copied space.)

Thanks for letting me know about the issue!
Comment 9 Andreas Kling 2015-11-13 11:23:14 PST
Created attachment 265485 [details]
Follow-up patch to fix 32-bit platforms
Comment 10 Mark Lam 2015-11-13 11:25:45 PST
Comment on attachment 265485 [details]
Follow-up patch to fix 32-bit platforms

r=me
Comment 11 Andreas Kling 2015-11-13 14:16:54 PST
Comment on attachment 265485 [details]
Follow-up patch to fix 32-bit platforms

Clearing flags on attachment: 265485

Committed r192443: <http://trac.webkit.org/changeset/192443>
Comment 12 Andreas Kling 2015-11-13 14:17:01 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 WebKit Commit Bot 2015-11-13 17:14:44 PST
Re-opened since this is blocked by bug 151285
Comment 14 Andreas Kling 2015-11-17 13:13:16 PST
Created attachment 265702 [details]
Patch for relanding

This had a subtle problem on 32-bit, where WriteBarrier<Unknown> is 64-bit and WriteBarrier<JSString> is 64-bit, and so you really can't go around reinterpret_casting one to the other!
Comment 15 Andreas Kling 2015-11-17 13:13:44 PST
(In reply to comment #14)
> Created attachment 265702 [details]
> Patch for relanding
> 
> This had a subtle problem on 32-bit, where WriteBarrier<Unknown> is 64-bit
> and WriteBarrier<JSString> is 64-bit, and so you really can't go around
> reinterpret_casting one to the other!

I meant to say that WriteBarrier<Unknown> is 64-bit, and WriteBarrier<JSString> is 32-bit. :)
Comment 16 WebKit Commit Bot 2015-11-17 14:13:45 PST
Comment on attachment 265702 [details]
Patch for relanding

Clearing flags on attachment: 265702

Committed r192536: <http://trac.webkit.org/changeset/192536>
Comment 17 WebKit Commit Bot 2015-11-17 14:13:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 WebKit Commit Bot 2015-11-24 13:35:14 PST
Re-opened since this is blocked by bug 151593