Bug 79608 – Getting the instruction stream for a code block should not require two loads

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

It should be one load at most.  Maybe someday we can even get it down to zero loads (i.e. the byte code is just appended to the CodeBlock).

[Filip Pizlo]

Created attachment 128927 [details]
the patch

[WebKit Review Bot]

Attachment 128927 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/bytecompiler/Label.h:49:  The parameter name "location" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/wtf/CompactVector.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Oliver Hunt]

Comment on attachment 128927 [details]
the patch

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

> Source/JavaScriptCore/wtf/CompactVector.h:40
> +// ** This could be modified to support non-POD values quite easily. It just
> +//    hasn't been, so far, because there has been no need.

Seems like it would be worth adding COMPILE_ASSERTs to prevent usage of non-POD

> Source/JavaScriptCore/wtf/CompactVector.h:71
> +        for (unsigned i = other.size(); i--;)
> +            m_data[i] = other[i];

Why not just use memcpy here? Also I'm not 100% comfortable with the implicit vector copy -- at the very least can we make these contractors explicit?

[Filip Pizlo]

(In reply to comment #3)
> (From update of attachment 128927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128927&action=review
> 
> > Source/JavaScriptCore/wtf/CompactVector.h:40
> > +// ** This could be modified to support non-POD values quite easily. It just
> > +//    hasn't been, so far, because there has been no need.
> 
> Seems like it would be worth adding COMPILE_ASSERTs to prevent usage of non-POD

Yeah, I'll do that.

> 
> > Source/JavaScriptCore/wtf/CompactVector.h:71
> > +        for (unsigned i = other.size(); i--;)
> > +            m_data[i] = other[i];
> 
> Why not just use memcpy here? Also I'm not 100% comfortable with the implicit vector copy -- at the very least can we make these contractors explicit?

You're right, this should be an explicit contractor.

(Contractor is the new constructor.)

[Filip Pizlo]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 128927 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=128927&action=review
> > 
> > > Source/JavaScriptCore/wtf/CompactVector.h:40
> > > +// ** This could be modified to support non-POD values quite easily. It just
> > > +//    hasn't been, so far, because there has been no need.
> > 
> > Seems like it would be worth adding COMPILE_ASSERTs to prevent usage of non-POD
> 
> Yeah, I'll do that.

Scratch that.  JSC::Instruction is not POD.  I mean, it is for our purposes, but Clang disagrees.

I'd rather not spend too much time thinking about what assertions to add to this class given that we'll rapidly end up in weirdo C++ type land.  What matters is that the class does exactly what it is supposed to do for byte code sequences.

> 
> > 
> > > Source/JavaScriptCore/wtf/CompactVector.h:71
> > > +        for (unsigned i = other.size(); i--;)
> > > +            m_data[i] = other[i];
> > 
> > Why not just use memcpy here? Also I'm not 100% comfortable with the implicit vector copy -- at the very least can we make these contractors explicit?
> 
> You're right, this should be an explicit contractor.
> 
> (Contractor is the new constructor.)

[Filip Pizlo]

Created attachment 128929 [details]
the patch

Changed CompactVector(const Vector&) constructor to be explicit, and to use memcpy.

Tried, and failed, to add static asserts for T being POD.  I failed because JSC::Instruction is not POD according to Clang, but we use it as if it were POD.

[Philippe Normand]

Comment on attachment 128929 [details]
the patch

Attachment 128929 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11630765

[Early Warning System Bot]

Comment on attachment 128929 [details]
the patch

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

[Gyuyoung Kim]

Comment on attachment 128929 [details]
the patch

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

[Filip Pizlo]

Created attachment 128930 [details]
the patch

[Sam Weinig]

Comment on attachment 128930 [details]
the patch

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

In addition to the rename and comment fix.  Please consider adding basic unit tests to TestWebKitAPI.

> Source/JavaScriptCore/ChangeLog:16
> +        This patch also gets rid of the bytecode discarding logic, since we don't
> +        use it anymore and it's unlikely to ever work right with DFG or LLInt. And
> +        I didn't feel like porting dead code to use CompactVector.

Sadness.

> Source/JavaScriptCore/wtf/CompactVector.h:33
> +// This implements a resizeable array for POD** values, which is optimized for:

This isn't actually resizable.

> Source/JavaScriptCore/wtf/CompactVector.h:47
> +template<typename T>
> +class CompactVector {

I think this name is misleading.  Really, this is just an array with an efficiently RefCounted data part.  RefCountedArray?

[Filip Pizlo]

Created attachment 128935 [details]
the patch

Renamed and fixed comment.  Will see what EWS thinks.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/108943