Closed
Bug 987379
Opened 10 years ago
Closed 10 years ago
The enableSPSProfilingAssertions function is confusing
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: gkw, Assigned: djvj)
References
Details
Attachments
(3 files, 3 obsolete files)
1.01 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
4.36 KB,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
770 bytes,
patch
|
h4writer
:
review+
|
Details | Diff | Splinter Review |
So, enableSPSProfilingAssertions(true) does not turn on SPS assertions, instead, it turns on the slow SPS assertions, while also turning on the profiler. enableSPSProfilingAssertions(false) does not turn off SPS assertions, instead, it turns off the slow SPS assertions, keeping the fast SPS assertions turned on (?). This seems incredibly confusing, I spoke to Kannan about this f2f at the JS work week, and he suggests to file this bug.
Assignee | ||
Comment 1•10 years ago
|
||
This changes the naming of enableSPSProfilingAssertions, and removes disableSPSProfiling. There is a single new function, with the following behaviour: enableSPSProfiling([enable=true], [slow=false]) All call variants possible: enableSPSProfiling() - enables profiling (with slow assertions off) enableSPSProfiling(false) - disables profiling enableSPSProfiling(true) - enables profiling (with slow assertions off) enableSPSProfiling(true, false) - enables profiling (with slow assertions off) enableSPSProfiling(true, true) - enables profiling (with slow assertions on) This seems like a more reasonable interface to toggling the profiler compared to the existing mechanism. What do you think?
Attachment #8397129 -
Flags: feedback?(jruderman)
Attachment #8397129 -
Flags: feedback?(choller)
Assignee | ||
Comment 2•10 years ago
|
||
Comment 3•10 years ago
|
||
Comment on attachment 8397129 [details] [diff] [review] fix-bug-987379.patch Sounds good to me :)
Attachment #8397129 -
Flags: feedback?(choller) → feedback+
Assignee | ||
Updated•10 years ago
|
Attachment #8397129 -
Flags: review?(hv1989)
Assignee | ||
Updated•10 years ago
|
Attachment #8397130 -
Flags: review?(hv1989)
Comment 4•10 years ago
|
||
> enableSPSProfiling() - enables profiling (with slow assertions off)
> enableSPSProfiling(false) - disables profiling
> enableSPSProfiling(true) - enables profiling (with slow assertions off)
> enableSPSProfiling(true, false) - enables profiling (with slow assertions
> off)
> enableSPSProfiling(true, true) - enables profiling (with slow assertions on)
That's awful. Boolean args are an anti-pattern in general. Pairs of related booleans (with some combinations being meaningless) are even worse.
There are only three possibilities. Make that clear.
- disableSPSProfiling()
- enableSPSProfilingWithSlowAssertions()
- enableSPSProfilingWithoutSlowAssertions()
No comments are necessary.
Reporter | ||
Comment 5•10 years ago
|
||
> There are only three possibilities. Make that clear.
>
> - disableSPSProfiling()
> - enableSPSProfilingWithSlowAssertions()
> - enableSPSProfilingWithoutSlowAssertions()
With this proposal, will a js shell which has executed:
enableSPSProfilingWithSlowAssertions()
to turn on slow assertions, then runs:
enableSPSProfilingWithoutSlowAssertions()
to turn off slow assertions successfully later?
Comment 6•10 years ago
|
||
I don't know. My point is that boolean arguments are the problem. Especially when you have a function called "enableFoo" and it can actually disable Foo. Figure out the necessary transitions, come up with appropriate functions that make those transitions clear, and don't allow redundant and/or meaningless combinations.
Comment 7•10 years ago
|
||
Comment on attachment 8397129 [details] [diff] [review] fix-bug-987379.patch Review of attachment 8397129 [details] [diff] [review]: ----------------------------------------------------------------- I think njn is right on the money and we should try to avoid boolean arguments.
Attachment #8397129 -
Flags: review?(hv1989)
Updated•10 years ago
|
Attachment #8397130 -
Flags: review?(hv1989)
Updated•10 years ago
|
Attachment #8397129 -
Flags: feedback?(jruderman)
Assignee | ||
Comment 8•10 years ago
|
||
Updated bug with new API.
Attachment #8397129 -
Attachment is obsolete: true
Attachment #8398736 -
Flags: review?(hv1989)
Assignee | ||
Comment 9•10 years ago
|
||
Fix tests.
Attachment #8397130 -
Attachment is obsolete: true
Attachment #8398740 -
Flags: review?(hv1989)
Assignee | ||
Comment 10•10 years ago
|
||
Minor change from previous patch to adopt simpler disable/re-enable logic.
Attachment #8398736 -
Attachment is obsolete: true
Attachment #8398736 -
Flags: review?(hv1989)
Comment 11•10 years ago
|
||
Comment on attachment 8398736 [details] [diff] [review] fix-bug-987379.patch Review of attachment 8398736 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! (Note: you did forget to put r? on this patch, but I assume this was by accident. Else I reviewed something too eagerly) ::: js/src/builtin/TestingFunctions.cpp @@ +978,5 @@ > + return true; > + > + // Slow assertions are on. Disable profiling before re-enabling > + // with slow assertions off. > + cx->runtime()->spsProfiler.enable(false); Since you also need to do this outside this "if(enabled())" for "if(installed())", you can remove this code. Installed() means enabled(). So the code outside the "if" that you need to add will trigger in that case too. So this can get removed... @@ +983,2 @@ > } > You need to code of: // Disable before re-enabling; see the assertion in |SPSProfiler::setProfilingStack|. if (cx->runtime()->spsProfiler.installed()) cx->runtime()->spsProfiler.enable(false); here too. @@ +1003,5 @@ > + return true; > + > + // Slow assertions are off. Disable profiling before re-enabling > + // with slow assertions on. > + cx->runtime()->spsProfiler.enable(false); No need to do this. Looking at the asserts in SPSProfiler::enabled(), enabled means installed. So the next code "if(installed())" will disable it, before enabling again.
Attachment #8398736 -
Flags: review+
Comment 12•10 years ago
|
||
Comment on attachment 8398740 [details] [diff] [review] fix-tests.patch Review of attachment 8398740 [details] [diff] [review]: ----------------------------------------------------------------- Rubber-stamp+
Attachment #8398740 -
Flags: review?(hv1989) → review+
Comment 13•10 years ago
|
||
Comment on attachment 8398746 [details] [diff] [review] fix-bug-987379.patch Review of attachment 8398746 [details] [diff] [review]: ----------------------------------------------------------------- Ok, a bit late. Looks like I reviewed the wrong patch. This also seems good ;). ::: js/src/builtin/TestingFunctions.cpp @@ +973,4 @@ > > // Disable before re-enabling; see the assertion in |SPSProfiler::setProfilingStack|. > if (cx->runtime()->spsProfiler.installed()) > cx->runtime()->spsProfiler.enable(false); Ok, this fixes my comment on the previous patch ;) @@ +997,5 @@ > + > + // Slow assertions are off. Disable profiling before re-enabling > + // with slow assertions on. > + cx->runtime()->spsProfiler.enable(false); > + } Ok, why not remove it here also. So why did you only remove it in the EnableSPSProfiling case?
Attachment #8398746 -
Flags: review+
Comment 14•10 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #13) > Ok, a bit late. Looks like I reviewed the wrong patch. This also seems good Any other patch I need to review. Now that I'm on a roll :P
Reporter | ||
Comment 15•10 years ago
|
||
Kannan, is this ready for landing soon?
Assignee: nobody → kvijayan
Status: NEW → ASSIGNED
Flags: needinfo?(kvijayan)
Assignee | ||
Comment 16•10 years ago
|
||
Thanks for reminding me. This is ready for landing, doing now.
Flags: needinfo?(kvijayan)
Assignee | ||
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/83010e4989ca
https://hg.mozilla.org/mozilla-central/rev/83010e4989ca
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 19•10 years ago
|
||
js/src/tests/js1_8_5/extensions/sps-generators.js fails with following error:
> sps-generators.js:26:2 ReferenceError: enableSPSProfilingAssertions is not defined
Assignee | ||
Comment 20•10 years ago
|
||
This is weird. I'm not sure why my try runs didn't catch this. Fixing now.
Updated•10 years ago
|
Attachment #8414647 -
Flags: review?(hv1989) → review+
Assignee | ||
Comment 22•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cecc0699b45
You need to log in
before you can comment on or make changes to this bug.
Description
•