Bug 79608

Summary: Getting the instruction stream for a code block should not require two loads
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gns, pnormand, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 79609    
Attachments:
Description Flags
the patch
none
the patch
pnormand: commit-queue-
the patch
sam: review+
the patch none

Description Filip Pizlo 2012-02-26 13:26:17 PST
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).
Comment 1 Filip Pizlo 2012-02-26 13:31:52 PST
Created attachment 128927 [details]
the patch
Comment 2 WebKit Review Bot 2012-02-26 13:34:39 PST
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.
Comment 3 Oliver Hunt 2012-02-26 14:00:27 PST
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?
Comment 4 Filip Pizlo 2012-02-26 14:03:14 PST
(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.)
Comment 5 Filip Pizlo 2012-02-26 14:22:35 PST
(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.)
Comment 6 Filip Pizlo 2012-02-26 14:25:44 PST
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.
Comment 7 Philippe Normand 2012-02-26 14:30:21 PST
Comment on attachment 128929 [details]
the patch

Attachment 128929 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11630765
Comment 8 Early Warning System Bot 2012-02-26 14:42:07 PST
Comment on attachment 128929 [details]
the patch

Attachment 128929 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11632729
Comment 9 Gyuyoung Kim 2012-02-26 14:46:36 PST
Comment on attachment 128929 [details]
the patch

Attachment 128929 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11627790
Comment 10 Filip Pizlo 2012-02-26 14:53:26 PST
Created attachment 128930 [details]
the patch
Comment 11 Sam Weinig 2012-02-26 15:48:03 PST
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?
Comment 12 Filip Pizlo 2012-02-26 16:53:24 PST
Created attachment 128935 [details]
the patch

Renamed and fixed comment.  Will see what EWS thinks.
Comment 13 Filip Pizlo 2012-02-26 18:08:22 PST
Landed in http://trac.webkit.org/changeset/108943