Bug 155336

<rdar://problem/25098147>

Created attachment 273725 [details] proposed patch. Still need to run benchmarks. This patch has passed the JSC and layout tests.

Comment on attachment 273725 [details] proposed patch. r=me

Comment on attachment 273725 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=273725&action=review > Source/JavaScriptCore/runtime/Executable.cpp:513 > + m_classSource = unlinkedExecutable->classSource(); Is this correct with code caching? Is it possible to construct a conflict here where we get instantiated with an unlinked code block that isn't from a class constructor?

Comment on attachment 273725 [details] proposed patch. Let me double check the UnlinkedFunctionExecutable caching issue.

To me it seems like we could construct the same exact normal "function(){...}" function, call new on it, and then have a class' constructor be the same source code, and get a conflict. I don't remember exactly how the hash is determined, but it seems possible that this could happen.

(In reply to comment #6) > To me it seems like we could construct the same exact > normal "function(){...}" function, call new on it, and then > have a class' constructor be the same source code, and get a > conflict. I don't remember exactly how the hash is determined, > but it seems possible that this could happen. Or even two different classes with the same constructor

(In reply to comment #7) > (In reply to comment #6) > > To me it seems like we could construct the same exact > > normal "function(){...}" function, call new on it, and then > > have a class' constructor be the same source code, and get a > > conflict. I don't remember exactly how the hash is determined, > > but it seems possible that this could happen. > > Or even two different classes with the same constructor I'm aware of the theoretical issue, but testing shows that there's no issue. I'll dig into the code a bit and will update later with reasons why there is / isn't an issue.

Comment on attachment 273725 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=273725&action=review > Source/JavaScriptCore/parser/ParserFunctionInfo.h:46 > const Identifier* className = 0; nullptr > Source/JavaScriptCore/parser/ParserFunctionInfo.h:50 > + unsigned startOffset = 0; > + unsigned endOffset = 0; > + int startLine = 0; > + unsigned startColumn = 0; In other WebKit code we have been using the brace syntax: unsigned startOffset { 0 }; Not sure it’s better, but it would be nice to be consistent in style.

Created attachment 273755 [details] proposed patch with more tests, some additional comments, + darin's feedback. I investigated the potential code caching issue, and concluded that there is no issue. This is because we currently only do caching of unlinked executables for global programs or dynamically created functions via the Function constructor. Class constructors are not cached, and hence, it is safe to stash the class source in its unlinked function executable. function-toString-vs-name.js already tests for this caching behavior, and I've also added some additional tests to check for global, eval, and Function constructor cases in this revised patch. If the code caching behavior changes in the future, the tests in function-toString-vs-name.js will catch it.

Created attachment 273757 [details] diff of the old patch vs the current one: for your reviewing convenience.

Comment on attachment 273755 [details] proposed patch with more tests, some additional comments, + darin's feedback. Saam tells me (offline) that he doesn't think this need additional review. Will land.

Thanks for all the reviews. Landed in r198042: <http://trac.webkit.org/r198042>.

*** Bug 144285 has been marked as a duplicate of this bug. ***

*** Bug 149743 has been marked as a duplicate of this bug. ***

*** Bug 149743 has been marked as a duplicate of this bug. ***