Static size inference for JavaScript objects

[Geoffrey Garen]

Static size inference for JavaScript objects

[Geoffrey Garen]

Created attachment 185041 [details]
Patch

[Early Warning System Bot]

Comment on attachment 185041 [details]
Patch

Attachment 185041 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16165688

[Early Warning System Bot]

Comment on attachment 185041 [details]
Patch

Attachment 185041 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16165687

[Filip Pizlo]

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).

[Build Bot]

Comment on attachment 185041 [details]
Patch

Attachment 185041 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16155740

[Build Bot]

Comment on attachment 185041 [details]
Patch

Attachment 185041 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16180219

[EFL EWS Bot]

Comment on attachment 185041 [details]
Patch

Attachment 185041 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16180241

[Build Bot]

Comment on attachment 185041 [details]
Patch

Attachment 185041 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16184074

[Geoffrey Garen]

> 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...

[Geoffrey Garen]

Created attachment 185119 [details]
Patch

[Build Bot]

Comment on attachment 185119 [details]
Patch

Attachment 185119 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16182227

[Geoffrey Garen]

Created attachment 185134 [details]
Patch

[Build Bot]

Comment on attachment 185134 [details]
Patch

Attachment 185134 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16156963

[Geoffrey Garen]

Created attachment 185149 [details]
Patch

[Geoffrey Garen]

Committed r141050: <http://trac.webkit.org/changeset/141050>

[Geoffrey Garen]

Comment on attachment 185149 [details]
Patch

Phil Pizlo reviewed this.

[Geoffrey Garen]

Created attachment 185298 [details]
Benchmark results