Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX backburner] Avoids spinning up unnecessary run loops via run.join #4698

Merged
merged 1 commit into from
Dec 9, 2016

Conversation

runspired
Copy link
Contributor

@runspired runspired commented Dec 6, 2016

Notes:

  • was split off from the micro-queue work in [PERF] use micro-queue #4668 so we could benchmark separately
  • Usage this replaces was a mis-use of run.join
  • This fix doesn't show any improvement in benchmark range at a glance
  • But the fix does tighten the cluster of values within the range in a good direction (so with proper outlier filtering we'd likely see a small gain)
  • The fix cleans up timeline charts significantly by coalescing the related work underneath one run.join call instead of it being split across several, which is it's own win
  • Benchmark is running in Chrome only, but this may improve Firefox more than Chrome.

Numbers are measuring store._push for 238 records of a single type, in milliseconds and from 35 runs comparing current master to this PR.

current master

{
  "min": "17.23",
  "q1": "18.77",
  "q2": "20.63",
  "q3": "22.27",
  "max": "36.74"
}

PR

{
  "min": "15.45",
  "q1": "18.61",
  "q2": "20.47",
  "q3": "22.60",
  "max": "34.13"
}

cc @stefanpenner @krisselden

Edit

I realized I should have been benchmarking against the complex scenario in which we actually have relationships. The complex scenario loads 34 primary records with 6 related record per primary record, for the same total of 238 records. New numbers below, before going "wait, where's the improvement?!" please see updated analysis in the related PR #4668

current master

{
  "min": "17.30",
  "q1": "23.76",
  "q2": "24.86",
  "q3": "27.17",
  "max": "40.92"
}

PR

{
  "min": "18.42",
  "q1": "22.95",
  "q2": "25.32",
  "q3": "27.20",
  "max": "38.13"
}

@runspired runspired mentioned this pull request Dec 6, 2016
@stefanpenner
Copy link
Member

But the fix does tighten the cluster of values within the range in a good direction (so with proper outlier filtering we'd likely see a small gain)

I don't think this has much statistical value, for the most part outliers can be largely ignored (on the basis that our environment is very noisy, and not totally isolated). That is Unless they happen to consistently and in non trivial ways skew the distribution.

My feeling is that for these

@stefanpenner
Copy link
Member

The fix cleans up timeline charts significantly by coalescing the related work underneath one run.join call instead of it being split across several, which is it's own win

This though, I think has value. Both in cleaning up the timeline, grouping similar work together.

As long as this isn't a breaking change (at first glance it doesn't look like it). It sounds like a good change!

@stefanpenner
Copy link
Member

I'm wondering if the 5 number summary gets abit too murky when dealing with less diverse distribution comparisons. There are other approaches that we can consider when comparing. Such as normalizing then comparing, rather then comparing non-normalized distributions. Typical Gaussian normalization can hide skew so it is absolutely not perfect, as nearly nothing is actually distributed this way. More often real world distributions are log normal distributed...

Ultimately statistics is silly:p. But it's likely the only tool we have for this sort of thing.

Maybe at some point we sit down and brainstorm abit. My goal here isn't to be pedantic, but to try to avoid common pitfalls us humans run into when our brain tries to reason about these sorts of things.

@bmac
Copy link
Member

bmac commented Dec 6, 2016

Overall this change looks good. 👍

@@ -2270,37 +2270,36 @@ Store = Service.extend({
*/
_push(data) {
let token = heimdall.start('store._push');
let included = data.included;
let i, length;
let ret = this._backburner.join(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename this back to internalModel

@igorT igorT merged commit 4ca90fa into emberjs:master Dec 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants