Bug 108093

Summary: Static size inference for JavaScript objects
Product: WebKit Reporter: Geoffrey Garen <ggaren>
Component: New BugsAssignee: Geoffrey Garen <ggaren>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, buildbot, dgrogan, fpizlo, jsbell, mhahnenberg, oliver, philn, rniwa, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
ggaren: review+
Benchmark results none

Description Geoffrey Garen 2013-01-28 11:02:39 PST
Static size inference for JavaScript objects
Comment 1 Geoffrey Garen 2013-01-28 13:02:29 PST
Created attachment 185041 [details]
Patch
Comment 2 Early Warning System Bot 2013-01-28 13:11:38 PST
Comment on attachment 185041 [details]
Patch

Attachment 185041 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16165688
Comment 3 Early Warning System Bot 2013-01-28 13:12:11 PST
Comment on attachment 185041 [details]
Patch

Attachment 185041 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16165687
Comment 4 Filip Pizlo 2013-01-28 13:16:09 PST
Comment on attachment 185041 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:128
> +        (JSC::DFG::AbstractState::execute): Upated for rename.

Typo "Upated"

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:757
> +UnlinkedValueProfile BytecodeGenerator::emitProfiledOpcode(int dst, OpcodeID opcodeID)
>  {
> +    m_staticPropertyAnalyzer.kill(dst);

Seems like it would be good to change the name of emitProfiledOpcode, since it does things to staticPropertyAnalyzer now.  Not sure what a good name would be.

> Source/JavaScriptCore/bytecompiler/StaticPropertyAnalyzer.h:38
> +// Used for flow-insensitive static analysis of the number of properties assigned to an object.
> +class StaticPropertyAnalyzer {
> +public:
> +    StaticPropertyAnalyzer(Vector<UnlinkedInstruction>*);
> +

Is it the case that this analysis can be wrong, and even if it is, the program will execute "correctly" albeit possibly less efficiently?

If so, then you should add a comment about this.  It's useful to be able to remember which analyses have to be sound at any cost (DFG::AbstractState) and which can be wrong (this one).
Comment 5 Build Bot 2013-01-28 13:28:47 PST
Comment on attachment 185041 [details]
Patch

Attachment 185041 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16155740
Comment 6 Build Bot 2013-01-28 14:13:14 PST
Comment on attachment 185041 [details]
Patch

Attachment 185041 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16180219
Comment 7 EFL EWS Bot 2013-01-28 14:59:36 PST
Comment on attachment 185041 [details]
Patch

Attachment 185041 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16180241
Comment 8 Build Bot 2013-01-28 17:10:33 PST
Comment on attachment 185041 [details]
Patch

Attachment 185041 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16184074
Comment 9 Geoffrey Garen 2013-01-28 17:47:23 PST
> Typo "Upated"

Fixed.

> Seems like it would be good to change the name of emitProfiledOpcode, since it does things to staticPropertyAnalyzer now.  Not sure what a good name would be.

This convinced me that I should just use a separate helper function instead. The fact that it corresponds to place where we call emitProfiledOpcode is just a coincidence, I guess.

> Is it the case that this analysis can be wrong, and even if it is, the program will execute "correctly" albeit possibly less efficiently?

Yes.

> If so, then you should add a comment about this.

Okeedokee.

Looking at the EWS failures now...
Comment 10 Geoffrey Garen 2013-01-28 17:55:50 PST
Created attachment 185119 [details]
Patch
Comment 11 Build Bot 2013-01-28 18:23:20 PST
Comment on attachment 185119 [details]
Patch

Attachment 185119 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16182227
Comment 12 Geoffrey Garen 2013-01-28 19:47:27 PST
Created attachment 185134 [details]
Patch
Comment 13 Build Bot 2013-01-28 21:19:49 PST
Comment on attachment 185134 [details]
Patch

Attachment 185134 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16156963
Comment 14 Geoffrey Garen 2013-01-28 21:35:58 PST
Created attachment 185149 [details]
Patch
Comment 15 Geoffrey Garen 2013-01-28 21:49:24 PST
Committed r141050: <http://trac.webkit.org/changeset/141050>
Comment 16 Geoffrey Garen 2013-01-29 13:33:49 PST
Comment on attachment 185149 [details]
Patch

Phil Pizlo reviewed this.
Comment 17 Geoffrey Garen 2013-01-29 13:34:14 PST
Created attachment 185298 [details]
Benchmark results