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).
Created attachment 128927 [details] the patch
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 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?
(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.)
(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.)
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 on attachment 128929 [details] the patch Attachment 128929 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11630765
Comment on attachment 128929 [details] the patch Attachment 128929 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11632729
Comment on attachment 128929 [details] the patch Attachment 128929 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11627790
Created attachment 128930 [details] the patch
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?
Created attachment 128935 [details] the patch Renamed and fixed comment. Will see what EWS thinks.
Landed in http://trac.webkit.org/changeset/108943