1206596 - [jsdbg2] Use FastBernoulliTrial class to control object allocation site tracking

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

In preparation for supporting string allocation site tracking, I've abstracted out the Bernoulli trial logic into its own MFBT class, implemented in bug 1206357. Our object allocation site tracking should use this class, instead of integrating the logic into the SavedStacks class.

Comment 1

[Jim Blandy :jimb]

Attachment: Make random_generateSeed visible, and let it generate wider seeds.

Comment 2

[Jim Blandy :jimb]

Attachment: Change js::SavedStacks to use mozilla::FastBernoulliTrial.

Comment 3

[Jim Blandy :jimb]

This patch depends on the refactoring implemented in bug 1206594.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8663515 [details] [diff] [review]
Change js::SavedStacks to use mozilla::FastBernoulliTrial.

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

\o/

::: js/src/builtin/TestingFunctions.cpp
@@ +858,5 @@
>      int32_t seed;
>      if (!ToInt32(cx, args[0], &seed))
>          return false;
>  
> +    cx->compartment()->savedStacks().setRNGState(seed, seed * 33);

Yay! Less magical bitwise arithmetic incantations!

But... what is 33?

I guess it doesn't matter that much for testing functions?

::: js/src/vm/SavedStacks.cpp
@@ +1331,5 @@
>  
>          if ((*dbgp)->trackingAllocationSites && (*dbgp)->enabled) {
>              foundAnyDebuggers = true;
> +            probability = std::max((*dbgp)->allocationSamplingProbability,
> +                                   probability);

Aside: this whole for loop makes me think we should have a

  template <class Iterable, class Initial, class F>
  Reduce(Iterable& iter, Initial val, F f);

template. It /should/ be relatively easy, fast, generic, and expressive now with lambdas, range-based-for-loops and the inlining implied/enabled by templates, right?

/me wonders how much push back this style might beget...

::: js/src/vm/SavedStacks.h
@@ +152,5 @@
>      friend bool JS::ubi::ConstructSavedFrameStackSlow(JSContext* cx,
>                                                        JS::ubi::StackFrame& ubiFrame,
>                                                        MutableHandleObject outSavedFrameStack);
>  
> +

Nit: I don't think we need this extra line.

@@ +179,1 @@
>      bool            creatingSavedFrame;

Nit: can you either align all of these members' names again or revert them all to one space after the type?

Comment 5

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a4b512df31d589ff52000cd172be8eb8cf17851
Bug 1206596: Make random_generateSeed visible, and let it generate wider seeds. r=fitzgen

https://hg.mozilla.org/integration/mozilla-inbound/rev/df0f9214b22402bd7badcd4d3547da1e3bf7ff5a
Bug 1206596: Change js::SavedStacks to use mozilla::FastBernoulliTrial. r=fitzgen

Comment 6

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c9a1a3a654eddc1df3506406564e0221ec033bf
Bug 1206596: Address review nits. DONTBUILD r=fitzgen

Comment 7

[Jim Blandy :jimb]

Attachment: Address review nits. DONTBUILD r=fitzgen

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a4b512df31d
https://hg.mozilla.org/mozilla-central/rev/df0f9214b224
https://hg.mozilla.org/mozilla-central/rev/0c9a1a3a654e