987379 - The enableSPSProfilingAssertions function is confusing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: fix-bug-987379.patch

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?

Comment 2

[Kannan Vijayan [:djvj]]

Attachment: fix-tests.patch

Comment 3

[Christian Holler (:decoder)]

Comment on attachment 8397129 [details] [diff] [review]
fix-bug-987379.patch

Sounds good to me :)

Comment 4

[Nicholas Nethercote [inactive]]

> 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.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> 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

[Nicholas Nethercote [inactive]]

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

[Hannes Verschore [:h4writer]]

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.

Comment 8

[Kannan Vijayan [:djvj]]

Attachment: fix-bug-987379.patch

Updated bug with new API.

Comment 9

[Kannan Vijayan [:djvj]]

Attachment: fix-tests.patch

Fix tests.

Comment 10

[Kannan Vijayan [:djvj]]

Attachment: fix-bug-987379.patch

Minor change from previous patch to adopt simpler disable/re-enable logic.

Comment 11

[Hannes Verschore [:h4writer]]

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.

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8398740 [details] [diff] [review]
fix-tests.patch

Review of attachment 8398740 [details] [diff] [review]:
-----------------------------------------------------------------

Rubber-stamp+

Comment 13

[Hannes Verschore [:h4writer]]

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?

Comment 14

[Hannes Verschore [:h4writer]]

(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

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Kannan, is this ready for landing soon?

Comment 16

[Kannan Vijayan [:djvj]]

Thanks for reminding me.  This is ready for landing, doing now.

Comment 17

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83010e4989ca

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/83010e4989ca

Comment 19

[Tooru Fujisawa [:arai]]

js/src/tests/js1_8_5/extensions/sps-generators.js fails with following error:
> sps-generators.js:26:2 ReferenceError: enableSPSProfilingAssertions is not defined

Comment 20

[Kannan Vijayan [:djvj]]

This is weird.  I'm not sure why my try runs didn't catch this.  Fixing now.

Comment 21

[Kannan Vijayan [:djvj]]

Attachment: fix-test.patch

Quick test fix.

Comment 22

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cecc0699b45

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2cecc0699b45