Rename useCount to warmupCounter

RESOLVED FIXED in mozilla35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nbp, Assigned: nbp)

Tracking

unspecified
mozilla35
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8485259 [details] [diff] [review]
Rename useCount to warmupCounter.
Attachment #8485259 - Flags: review?(hv1989)
(Assignee)

Comment 2

4 years ago
Created attachment 8485260 [details] [diff] [review]
Rename usesBefore* to *WarmupThreshold.
Attachment #8485260 - Flags: review?(hv1989)
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/
Attachment #8485260 - Flags: review?(hv1989)
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
Attachment #8485259 - Flags: review?(hv1989) → review+
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.
Attachment #8485260 - Flags: review+
(Assignee)

Comment 6

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/64203c2e785d
https://hg.mozilla.org/mozilla-central/rev/891d587c19c4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Created attachment 8487835 [details] [diff] [review]
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.
Attachment #8487835 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 9

4 years ago
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.
Attachment #8487835 - Flags: review?(nicolas.b.pierron)
Attachment #8487835 - Flags: review?(mrosenberg)
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.
Attachment #8487835 - Flags: review+
Comment on attachment 8487835 [details] [diff] [review]
Fixes

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

I think one r+ of marty is enough ;)
Attachment #8487835 - Flags: review?(mrosenberg)
You need to log in before you can comment on or make changes to this bug.