Removed unnecessary calls to _recompute_all_models() to speed up adding roots by EfremBraun · Pull Request #11739 · bokeh/bokeh

EfremBraun · 2021-10-13T21:34:20Z

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

…ng roots

EfremBraun · 2021-10-14T15:25:48Z

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...

…recompute_all_models(), and that is only worked around in special situations.

EfremBraun · 2021-10-14T15:49:32Z

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 · 2021-10-14T16:22:57Z

@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 · 2021-10-14T16:50:06Z

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 · 2021-10-14T16:54:48Z

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 · 2021-10-14T16:55:15Z

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 · 2021-10-14T16:55:55Z

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 · 2021-10-14T16:58:26Z

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 · 2021-10-14T17:05:03Z

It's possible there was a chrome update

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

g-parki · 2021-10-19T22:18:56Z

@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):

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

EfremBraun · 2021-10-19T22:48:52Z

Done, though I used:

git fetch upstream
git merge upstream/branch-3.0 branch-3.0
git push origin branch-3.0

bryevdv · 2021-10-22T16:30:28Z

Thanks for the PR @EfremBraun !

…ng roots (#11739) * Removed unnecessary calls to _recompute_all_models() to speed up adding roots * Corrected last commit so that general calls to add_root() still call recompute_all_models(), and that is only worked around in special situations. * Wrapping calls to add_root() within push_all_models_freeze() * Made last commit more elegant by getting rid of loop Co-authored-by: Efrem Braun <efrem.braun@gmail.comm>

* Update CONTRIBUTING.MD (#11957) * track modified files in release build more carefully (#11972) * Remove katex example (#11894) * Remove katex example * Remove test based on katex example * Revert "Remove test based on katex example" This reverts commit f0df5ed. * Cleaner white space/indentation in templates (#11866) * Enable block trimming, add initial test * Add test for script start/stops * Trim comment blocks in templates * Remove hard-coded white space in templates * Minimize manual trimming/indenting * Clean up indentation, standardize CSS/JS * Simplify new template test * Expand new test to cover multiple resource modes * Reduce whitespace around title and scripts * isort, lint test_templates * Combine JS/CSS resource defs in test_templates * Reduce indent of plot script * Consistent indentation in template itself * Update template in docs * 11726 Updated links to visjs.org Graph3d examples (#11728) * Add sphinx_copybutton (#11993) * Add sphinx_copybutton * Update CI environments * Move sphinx-copybutton to pip dependencies * Fix typo in tools.py (#11935) perfrom -> perform * Fixed a few typos and changed wordings (#11698) * Fix issue probably-meant-fstring found at https://codereview.doctor (#12097) * add functools.wraps to _needs_document_lock_wrapper (#11976) dask.distributed intermittently logs: `RuntimeWarning: coroutine '_needs_document_lock.<locals>._needs_document_lock_wrapper' was never awaited` with this wrapper it will be possible to see which method wasn't awaited * Md files checking and updating (#11903) * Update readme Resolving issue [#11868] (#11868) * Update README.md * Update README.md * Update README.md * Update commits with comments from #11903 * Update according to #1193 comments * Update readme to comply code quality test * update readme to comply code quality issues * Elaborate on git commit/push process in docs (#11758) * Add more git instructions * Fix typo in models contributor guide * Update sphinx/source/docs/dev_guide/pull_requests.rst Co-authored-by: Timo Cornelius Metzger <39711796+tcmetzger@users.noreply.github.com> Co-authored-by: Timo Cornelius Metzger <39711796+tcmetzger@users.noreply.github.com> * Hover tooltips for Patch and Areas (#11992) * Initial functioning WIP * WIP Patch and HArea * check for potentially null value * Remove commented out code * remove unnecessary to_screen call * break after successful polygon hit test * Add a comment regarding selection.is_empty() Co-authored-by: Mateusz Paprocki <mattpap@gmail.com> * Remove references to gen.coroutine from docs and update without_document_lock decorator (#12010) * Update without_document_lock to decorate async functions and asyncio.coroutines as well * Update docs to use async functions instead of tornado.gen.coroutines * Add fact that async funcs are accepted in docstring * Add test for async func decorated by without_document_lock * Remove trailing whitespace * simplify CI jobs with composite action (#11833) * simplfy CI jobs with composite action * lower verbosity on unpack * Removed unnecessary calls to _recompute_all_models() to speed up adding roots (#11739) * Removed unnecessary calls to _recompute_all_models() to speed up adding roots * Corrected last commit so that general calls to add_root() still call recompute_all_models(), and that is only worked around in special situations. * Wrapping calls to add_root() within push_all_models_freeze() * Made last commit more elegant by getting rid of loop Co-authored-by: Efrem Braun <efrem.braun@gmail.comm> Co-authored-by: Timo Cornelius Metzger <39711796+tcmetzger@users.noreply.github.com> Co-authored-by: g-parki <61096711+g-parki@users.noreply.github.com> Co-authored-by: Bill <35750915+roadswitcher@users.noreply.github.com> Co-authored-by: Ikko Ashimine <eltociear@gmail.com> Co-authored-by: Harold <eldrickmargarejo@gmail.com> Co-authored-by: code-review-doctor <72647856+code-review-doctor@users.noreply.github.com> Co-authored-by: Thomas Grainger <tagrain@gmail.com> Co-authored-by: girolamo <58935223+girolamodaschio@users.noreply.github.com> Co-authored-by: Alex Pilon <alex.pilon@gmail.com> Co-authored-by: Mateusz Paprocki <mattpap@gmail.com> Co-authored-by: Ben Russell <bprussell80@protonmail.com> Co-authored-by: Efrem Braun <efrem.braun@gmail.com> Co-authored-by: Efrem Braun <efrem.braun@gmail.comm>

github-actions · 2024-10-26T10:10:10Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Removed unnecessary calls to _recompute_all_models() to speed up addi…

ee1fe68

…ng roots

Corrected last commit so that general calls to add_root() still call …

cb2b604

…recompute_all_models(), and that is only worked around in special situations.

Wrapping calls to add_root() within push_all_models_freeze()

598dc9e

Made last commit more elegant by getting rid of loop

4d81f21

Merge remote-tracking branch 'upstream/branch-3.0' into branch-3.0

1b4df42

bryevdv approved these changes Oct 22, 2021

View reviewed changes

bryevdv added the status: accepted label Oct 22, 2021

bryevdv added this to the 3.0 milestone Oct 22, 2021

bryevdv merged commit 12f4477 into bokeh:branch-3.0 Oct 22, 2021

bryevdv mentioned this pull request May 16, 2022

More 2.4.3 backports #12130

Merged

bryevdv modified the milestones: 3.0, 2.4.3 May 16, 2022

github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2024

Uh oh!

Conversation

EfremBraun commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EfremBraun commented Oct 14, 2021

Uh oh!

EfremBraun commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryevdv commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryevdv commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EfremBraun commented Oct 14, 2021

Uh oh!

EfremBraun commented Oct 14, 2021

Uh oh!

bryevdv commented Oct 14, 2021

Uh oh!

EfremBraun commented Oct 14, 2021

Uh oh!

bryevdv commented Oct 14, 2021

Uh oh!

g-parki commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EfremBraun commented Oct 19, 2021

Uh oh!

bryevdv commented Oct 22, 2021

Uh oh!

github-actions bot commented Oct 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EfremBraun commented Oct 13, 2021 •

edited

Loading

EfremBraun commented Oct 14, 2021 •

edited

Loading

bryevdv commented Oct 14, 2021 •

edited

Loading

bryevdv commented Oct 14, 2021 •

edited

Loading

g-parki commented Oct 19, 2021 •

edited

Loading