1063816 - Rename useCount to warmupCounter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

SpiderMonkey is using a field named useCount to check if the script has been exercised enough since we reset any valuable information needed for making efficient compilation.  As opposed to what the name suggest (counting the number of uses), it is a warm-up counter.

a use counter:
 - should only be incremented.

a warmup counter:
 - is incremented as we exercise "one" set of types. (this implies that each time we see a new type, we should probably reduce the warm-up counter)
 - is reset when we lose type information.
 - does not have to be incremented once we reach the last compilation stage.

Thus, I am suggesting renaming what we currently call useCount to warmupCounter in order to match the uses of it.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Rename useCount to warmupCounter.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Rename usesBefore* to *WarmupThreshold.

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8485260 [details] [diff] [review]
Rename usesBefore* to *WarmupThreshold.

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

Shouldn't this be WarmUpThreshold? So capital "U"?

::: js/src/jit/BaselineCompiler.cpp
@@ +679,5 @@
>  
>      Label skipCall;
>  
>      const OptimizationInfo *info = js_IonOptimizations.get(js_IonOptimizations.firstLevel());
> +    uint32_t minUses = info->compilerWarmupThreshold(script, pc);

s/minUses/warmupThreshold
s/compilerWarmupThreshold/ionWarmupThreshold

::: js/src/jit/BaselineJIT.cpp
@@ +267,5 @@
>      // rest of the function.
>      if (cx->runtime()->forkJoinWarmup > 0) {
>          if (osr)
>              return Method_Skipped;
> +    } else if (script->incWarmupCounter() <= js_JitOptions.baselineCompilerWarmupThreshold) {

baselineWarmupThreshold??

::: js/src/jit/IonOptimizationLevels.cpp
@@ +69,5 @@
>  
>      if (pc == script->code())
>          pc = nullptr;
>  
> +    uint32_t minUses = compilerWarmupThreshold_;

s/minUses/warmUpThreshold

@@ +70,5 @@
>      if (pc == script->code())
>          pc = nullptr;
>  
> +    uint32_t minUses = compilerWarmupThreshold_;
> +    if (js_JitOptions.forceDefaultIonCompilerWarmupThreshold)

s/forceDefaultIonCompilerWarmupThreshold/forceDefaultIonWarmupThreshold/

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8485259 [details] [diff] [review]
Rename useCount to warmupCounter.

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

s/warmup/warmUp/g for functions
s/warmup/warm-up/g for text

I mentioned a few, but stopped after some time. I think you get the picture ;).

::: js/src/gc/Zone.cpp
@@ +202,5 @@
>               */
>              jit::FinishDiscardBaselineScript(fop, script);
>  
>              /*
> +             * Warmup counter for scripts are reset on GC. After discarding code we

Warm-up or Warm up

::: js/src/jit-test/lib/jitopts.js
@@ +26,5 @@
>  }
>  
>  // N.B. Ion opts *must come before* baseline opts because there's some kind of
> +// "undo eager compilation" logic. If we don't set the baseline warmup-counter
> +// *after* the Ion warmup-counter we end up setting the baseline warmup-counter

warm-up counter

::: js/src/jit-test/tests/ion/testFloat32.js
@@ +51,5 @@
>  // not inlined and the compiler will consider the return value to be a double, not a float32, making the
>  // assertions fail. Note that as assertFloat32 is declared unsafe for fuzzing, this can't happen in fuzzed code.
>  //
> +// To be able to test it, we still need ion compilation though. A nice solution
> +// is to manually lower the ion warmup trigger.

warm-up

::: js/src/jit/BaselineCompiler.cpp
@@ +666,3 @@
>  
> +    // If this is a loop inside a catch or finally block, increment the warmup
> +    // counter but don't attempt OSR (Ion only compiles the try block).

warm-up

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8485260 [details] [diff] [review]
Rename usesBefore* to *WarmupThreshold.

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

r+ if you agree with my comments, otherwise please file another patch for review.

Comment 6

[Nicolas B. Pierron [:nbp]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7e134a1f7273

I approved and followed all requested modifications and verified locally that everything was still compiling after the rebase.

https://hg.mozilla.org/integration/mozilla-inbound/rev/64203c2e785d
https://hg.mozilla.org/integration/mozilla-inbound/rev/891d587c19c4

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64203c2e785d
https://hg.mozilla.org/mozilla-central/rev/891d587c19c4

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: Fixes

I think the s/// might have been a bit too harsh.
- jsopcodeinlines.h, GetUseCount shouldn't have been adjusted
- MDefinition::defUseCount() also shouldn't have been adjusted
- I modified some warmUpCounter to warmUpThreshold if appropiate.

Also for nitpicking I adjusted counter to count in some places. As in, if we want the number it is better to call it warmUpCount. If we are talking about this particular counter/mechanism and what it does, it should be warmUpCounter.

Comment 9

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8487835 [details] [diff] [review]
Fixes

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

::: js/src/jit/MIR.cpp
@@ +386,5 @@
>  }
>  
>  #ifdef DEBUG
>  size_t
> +MDefinition::useCount() const

yes for this one.

@@ +395,5 @@
>      return count;
>  }
>  
>  size_t
> +MDefinition::defUseCount() const

and this one.

::: js/src/jsscript.h
@@ +818,5 @@
>      /* Range of characters in scriptSource which contains this script's source. */
>      uint32_t        sourceStart_;
>      uint32_t        sourceEnd_;
>  
> +    uint32_t        warmUpCount; /* Number of times the script has been called

We should ask a native speaker.

Comment 10

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8487835 [details] [diff] [review]
Fixes

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

::: js/src/jsscript.h
@@ +818,5 @@
>      /* Range of characters in scriptSource which contains this script's source. */
>      uint32_t        sourceStart_;
>      uint32_t        sourceEnd_;
>  
> +    uint32_t        warmUpCount; /* Number of times the script has been called

A counter is an object or device for counting.  "The count" is a concrete number that has been counted.  For the getters, "count" is definitely correct.  For the underlying variable, an argument could be made in either direction:
uint32_t warmUpCount --  this is an integer, the concrete number of times this has been run
uint32_t warmUpCounter -- this is a variable, it holds an integer count, and is thus a counter (e.g. http://coachdavesports.com/metal_counter.jpg ).  My personal preference is the former, since an integer isn't a horribly good container or abstraction.  If it were an object (possibly a good idea, since we seem to be counting many things), then I'd advocate for the latter.

Comment 11

[Hannes Verschore [:h4writer]]

Comment on attachment 8487835 [details] [diff] [review]
Fixes

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

I think one r+ of marty is enough ;)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06eebd56579d