Removed unnecessary calls to _recompute_all_models() to speed up adding roots

[EfremBraun]

• issues: partially fixes Avoid expensive over-computing "all models" on Document init #11707

[EfremBraun]

Ah, I see the issue. The tests directly access BokehJS's add_root() in document.ts, whereas the change I made relies on add_root() being accessed indirectly and then calling recompute_all_models() afterwards. If add_root() is accessed directly, then recompute_all_models() never gets called.

Let me think of a more elegant way to address this...

[EfremBraun]

This is a bit of a janky fix...in order to avoid the calls to recompute_all_models() without disturbing the add_root() function, I made a modified add_root() function called add_root_no_recompute(), which only gets called in the one circumstance I want it to happen in.

It'd be nice if JS had a context manager similar to Python such that I could wrap the add_root() calls within a statement that prevents the final recompute_all_models() call until the end (similar to the freeze() function in https://github.com/bokeh/bokeh/blob/branch-3.0/bokeh/document/models.py), but I don't see how to do that. But a more experienced JS developer might have a more elegant idea.

[bryevdv]

@EfremBraun Thanks for posting a PR, it's easier for me to digest proposed changes by just seeing them as a diff. Now that I have, why don't we back up a step. The purpose of the push/pop freeze functions is itself to prevent over-(re-)computation. Why don't we use that facility as intended? There is a loop for adding roots, why not try to fence the original push/pop functions around the entire loop? That should be sufficient to prevent a re-computation on on the individual roots, since the freeze count would only reach zero after the loop.

[bryevdv]

FYI I was proposing

const doc = new Document({resolver})

doc._push_all_models_freeze()
for (const id of root_ids) {
  const root = references.get(id)
    if (root != null) {
      doc.add_root(root as Model)
    }
  }
}
doc._pop_all_models_freeze() // freeze count only reaches zero here

which I think should do what you are after?

[EfremBraun]

FYI I was proposing

const doc = new Document({resolver})

doc._push_all_models_freeze()
for (const id of root_ids) {
  const root = references.get(id)
    if (root != null) {
      doc.add_root(root as Model)
    }
  }
}
doc._pop_all_models_freeze() // freeze count only reaches zero here

which I think should do what you are after?

Oh, that works too. And yes, it looks a lot more elegant. I'll make the change now.

[EfremBraun]

Regardless though, it is going to fail the one BokehJS-CI test on Windows, where the bbox bounds sometimes get changed a little...I'm not so sure why that would be. Any idea for what might be happening and how it could be fixed?

[bryevdv]

I don't offhand the first thing I would suggest is to just make sure you are up to date (rebased) on the latest branch-3.0 If that's not the issue I will have to try to investigate more directly.

[EfremBraun]

I don't offhand the first thing I would suggest is to just make sure you are up to date (rebased) on the latest branch-3.0 If that's not the issue I will have to try to investigate more directly.

I was up-to-date. So it'd have to be something else...

[bryevdv]

It's possible there was a chrome update

cc @IuryPiva any ideas about baseline failures only on windows?

[g-parki]

@EfremBraun the Windows test issue is fixed now. Can you please rebase and force push? My typical way to do so (sorry if you're already familiar with this):

1. Update your Github repository
2. From your project folder, check out main branch: git checkout branch-3.0
3. Pull updates git pull
4. Rebase git rebase branch-3.0 your_branch_name
5. Checkout your branch git checkout your_branch_name
6. Push updates to Github git push -f

[EfremBraun]

Done, though I used:

1. git fetch upstream
2. git merge upstream/branch-3.0 branch-3.0
3. git push origin branch-3.0

[bryevdv]

Thanks for the PR @EfremBraun !