Closed
Bug 1329150
Opened 8 years ago
Closed 8 years ago
Remove ENABLE_ARM_LR_SAVING and its code
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: mstange, Assigned: jseward)
References
Details
Attachments
(1 file)
(deleted),
patch
|
mstange
:
review+
mstange
:
feedback+
|
Details | Diff | Splinter Review |
On ARM we put the value of the LR register into the profiler buffer but never exposed it through in the actual profile object (bug 788289). We should stop putting these values into the buffer.
Comment 1•8 years ago
|
||
Julian, want to take this one? Looks pretty trivial, just removing the #ifdef'd code.
Assignee: nobody → jseward
Flags: needinfo?(jseward)
Assignee | ||
Comment 2•8 years ago
|
||
Markus, I'm a bit confused about this. With ENABLE_ARM_LR_SAVING
enabled, we copy the link register into TickSample::lr, and that
is then "used" in two places:
(1) GeckoSampler.cpp, doSampleStackTrace():
#ifdef ENABLE_ARM_LR_SAVING
aProfile.addTag(ProfileEntry('L', (void*)aSample->lr));
#endif
(2) GeckoSampler.cpp, mergeStacksIntoProfile():
JS::ProfilingFrameIterator::RegisterState registerState;
registerState.pc = aSample->pc;
registerState.sp = aSample->sp;
#ifdef ENABLE_ARM_LR_SAVING
registerState.lr = aSample->lr;
#endif
Which of these uses do you want to remove? It seems to me that
we could remove (1), but removing (2) will mean we give the JS
world uninitialised junk in registerState.lr.
Should we enable ENABLE_ARM_LR_SAVING always (I mean, always
copy the value into TickSample), remove (1), but keep (2) ?
Or something else?
Flags: needinfo?(jseward) → needinfo?(mstange)
Reporter | ||
Comment 3•8 years ago
|
||
Oh, I hadn't seen (2)! I was referring to (1), because those profile entries are never consumed, and I thought (1) was the only user of the LR register values we could remove all of ENABLE_ARM_LR_SAVING.
I've found one user of ProfilingFrameIterator::RegisterState::lr, at http://searchfox.org/mozilla-central/rev/ed04f7a44925dace34f2d2e15ef9c0f2809d70b0/js/src/wasm/WasmFrameIterator.cpp#698 so it looks like we can't remove this after all.
We should still remove (1) though, i.e. stop creating 'L' ProfileEntries.
Flags: needinfo?(mstange)
Reporter | ||
Comment 4•8 years ago
|
||
(In reply to Markus Stange [:mstange] from comment #3)
> Oh, I hadn't seen (2)! I was referring to (1), because those profile entries
> are never consumed, and I thought (1) was the only user of the LR register
> values we could remove all of ENABLE_ARM_LR_SAVING.
Sorry for the broken sentence. I meant:
I thought (1) was the only user of the LR register values, so I thought we could remove all ENABLE_ARM_LR_SAVING code.
Assignee | ||
Comment 5•8 years ago
|
||
Proposed fix, as per comment #3.
Attachment #8829813 -
Flags: feedback?(mstange)
Reporter | ||
Comment 6•8 years ago
|
||
Comment on attachment 8829813 [details] [diff] [review]
bug1329150-1.diff
Review of attachment 8829813 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, sorry for the delay.
Attachment #8829813 -
Flags: feedback?(mstange) → feedback+
Assignee | ||
Comment 7•8 years ago
|
||
Marcus, could you please upgrade that to an r+ then? Sorry, I should
have asked for it directly.
Flags: needinfo?(mstange)
Reporter | ||
Comment 8•8 years ago
|
||
Comment on attachment 8829813 [details] [diff] [review]
bug1329150-1.diff
Review of attachment 8829813 [details] [diff] [review]:
-----------------------------------------------------------------
Oh, of course.
Attachment #8829813 -
Flags: review+
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb5373470c1
Remove ENABLE_ARM_LR_SAVING and its code. r=mstange.
Comment 10•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(mstange)
You need to log in
before you can comment on or make changes to this bug.
Description
•