Closed
Bug 1064668
Opened 10 years ago
Closed 10 years ago
fix two asm.js profiling regressions
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla35
Tracking | Status | |
---|---|---|
firefox33 | --- | unaffected |
firefox34 | + | fixed |
firefox35 | --- | fixed |
People
(Reporter: luke, Assigned: luke)
References
Details
Attachments
(4 files)
4.17 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
2.89 KB,
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
6.85 KB,
patch
|
Details | Diff | Splinter Review | |
3.11 KB,
patch
|
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
These came up profiling the SIMD demos.
Assignee | ||
Comment 1•10 years ago
|
||
Function offsets need to eventually be stored in the AsmJSModule relative to the beginning of the module. However, my earlier patch changed them to be module-relative too early and thus was feeding module-relative offsets into tokenStream.lineNum() which expects ScriptSource-relative offsets.
Assignee: nobody → luke
Attachment #8486176 -
Flags: review?(benj)
Assignee | ||
Comment 2•10 years ago
|
||
I think I suggested the "refactoring" that broke this. The issue is that AsmJSActivation contains fields that were being initialized after the activation had been added to rt->profilingActivation_ and thus visible to asynchronous profiling.
Attachment #8486177 -
Flags: review?(kvijayan)
Assignee | ||
Comment 3•10 years ago
|
||
Combined patch for Dan.
Comment 4•10 years ago
|
||
Comment on attachment 8486177 [details] [diff] [review] fix-asmjs-profiling Review of attachment 8486177 [details] [diff] [review]: ----------------------------------------------------------------- Ah, such finnicky stuff :)
Attachment #8486177 -
Flags: review?(kvijayan) → review+
Comment 5•10 years ago
|
||
Comment on attachment 8486176 [details] [diff] [review] fix-line-numbers Review of attachment 8486176 [details] [diff] [review]: ----------------------------------------------------------------- Nice catch! ::: js/src/asmjs/AsmJSValidate.cpp @@ -1018,5 @@ > - > - // The begin/end char range is relative to the beginning of the module. > - // hence the assertions. > - JS_ASSERT(fn->pn_pos.begin > m.srcStart()); > - JS_ASSERT(fn->pn_pos.begin <= fn->pn_pos.end); Could we keep these two assertions in AsmJSModule.h?
Attachment #8486176 -
Flags: review?(benj) → review+
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #5) > Could we keep these two assertions in AsmJSModule.h? Yes, good point, I'll put them in AsmJSModule::addExportedFunction. https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9c1780c779 https://hg.mozilla.org/integration/mozilla-inbound/rev/526c183e9cd3
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/526c183e9cd3 https://hg.mozilla.org/mozilla-central/rev/5d9c1780c779
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Assignee | ||
Comment 8•10 years ago
|
||
Approval Request Comment [Feature/regressing bug #]:1057082 [User impact if declined]: crash while profiling asm.js code [Describe test coverage new/current, TBPL]: m-c, m-a [Risks and why]: quite low; this is a pretty simple fix to a simple bug; no rebasing required.
Attachment #8506950 -
Flags: approval-mozilla-beta?
Updated•10 years ago
|
Updated•10 years ago
|
status-firefox33:
--- → unaffected
Comment 9•10 years ago
|
||
Comment on attachment 8506950 [details] [diff] [review] beta.patch Beta+
Attachment #8506950 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in
before you can comment on or make changes to this bug.
Description
•