Summary: | JSC interpreter regressions after r69940 | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peter Gal <galpeter> | ||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, oliver, ossy, zherczeg | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Peter Gal
2010-10-18 11:24:43 PDT
Bug: JSValue result = JSC::resolveBase(callFrame, ident, callFrame->scopeChain(), isStrictPut); if (!result) { callFrame->r(dst) = result; ASSERT(callFrame->r(dst).jsValue()); } else callFrame->globalData().exception = createErrorForInvalidGlobalAssignment(callFrame, ident.ustring()); "if (!result)" is wrong, because it should test the opposite. Although "if (!!result)" is ok, I refactored the code with "UNLIKELY(!result)". Created attachment 71403 [details]
patch
Comment on attachment 71403 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=71403&action=review > JavaScriptCore/interpreter/Interpreter.cpp:292 > - if (!result) { > - callFrame->r(dst) = result; > - ASSERT(callFrame->r(dst).jsValue()); > - } else > + if (UNLIKELY(!result)) { > callFrame->globalData().exception = createErrorForInvalidGlobalAssignment(callFrame, ident.ustring()); > + return; > + } > + callFrame->r(dst) = result; > + ASSERT(callFrame->r(dst).jsValue()); > } I don't think we need to change the order of cases. I prefer only fixing the conditon and add LIKELY hint to the compiler, like this: if (LIKELY(result)) { callFrame->r(dst) = result; ASSERT(callFrame->r(dst).jsValue()); } else callFrame->globalData().exception = createErrorForInvalidGlobalAssignment(callFrame, ident.ustring()); Created attachment 71414 [details]
Patch for Oszi
I don't like it. Anyway it works.
(In reply to comment #4) > Created an attachment (id=71414) [details] > Patch for Oszi > > I don't like it. Anyway it works. I do like it. :) Comment on attachment 71414 [details] Patch for Oszi Clearing flags on attachment: 71414 Committed r70299: <http://trac.webkit.org/changeset/70299> All reviewed patches have been landed. Closing bug. |