Bug 66265 - Move allocation in constructors into separate constructorBody() methods
Summary: Move allocation in constructors into separate constructorBody() methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks: 65347
  Show dependency treegraph
 
Reported: 2011-08-15 16:51 PDT by Mark Hahnenberg
Modified: 2011-08-18 17:58 PDT (History)
3 users (show)

See Also:


Attachments
Patch (44.48 KB, patch)
2011-08-15 16:55 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (43.66 KB, patch)
2011-08-15 17:03 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Change function name (44.58 KB, patch)
2011-08-17 12:44 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Fixing windows build errors (45.57 KB, patch)
2011-08-18 16:02 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2011-08-15 16:51:08 PDT
This is the next small step in the process of refactoring JSC constructors to do no allocation in the middle of the execution of an initialization list.  In this bug, we're going to pull the allocation done in the constructor bodies into separate finishAllocations() methods, which will in turn be called from the constructor bodies.  This will set us up to have those finishAllocations() methods called outside of the constructor, allowing us to avoid allocation during initialization.
Comment 1 Mark Hahnenberg 2011-08-15 16:55:40 PDT
Created attachment 103980 [details]
Patch
Comment 2 Mark Hahnenberg 2011-08-15 17:03:10 PDT
Created attachment 103982 [details]
Patch
Comment 3 Geoffrey Garen 2011-08-16 15:51:33 PDT
Comment on attachment 103982 [details]
Patch

Patch looks good.

I think the simplest idiom to understand and enforce is "no constructor bodies or non-trivial initializers in GC objects." Then, this extra function you've added functions as the constructor body. So, let's call it "constructorBody".

r=me with that change.
Comment 4 Mark Hahnenberg 2011-08-17 12:44:28 PDT
Created attachment 104216 [details]
Change function name
Comment 5 Mark Hahnenberg 2011-08-18 16:02:44 PDT
Created attachment 104419 [details]
Fixing windows build errors
Comment 6 Oliver Hunt 2011-08-18 16:08:15 PDT
Comment on attachment 104419 [details]
Fixing windows build errors

View in context: https://bugs.webkit.org/attachment.cgi?id=104419&action=review

> Source/JavaScriptCore/runtime/JSPropertyNameIterator.h:90
> +            PropertyNameArrayData::PropertyNameVector& propertyNameVector = propertyNameArrayData->propertyNameVector();

Out of curiosity, how does JSPropertyNameIterator handle gc during initialisation -- eg. if jsOwnedString triggers a gc?
Comment 7 Mark Hahnenberg 2011-08-18 16:15:33 PDT
> Out of curiosity, how does JSPropertyNameIterator handle gc during initialisation -- eg. if jsOwnedString triggers a gc?

Poorly I think.  visitChildren appends all of the values in m_jsStrings to the visitor, so if you're only halfway through creating the list, I think it has a potential to blow up.
Comment 8 WebKit Review Bot 2011-08-18 17:58:47 PDT
Comment on attachment 104419 [details]
Fixing windows build errors

Clearing flags on attachment: 104419

Committed r93378: <http://trac.webkit.org/changeset/93378>
Comment 9 WebKit Review Bot 2011-08-18 17:58:53 PDT
All reviewed patches have been landed.  Closing bug.