Page MenuHome

Performance: Remap multiple items in UI
ClosedPublic

Authored by Jeroen Bakker (jbakker) on Dec 17 2021, 5:06 PM.

Details

Summary

During sprite fright loading of complex scenes would spend a long time in remapping ID's
The remapping process is done on a per ID instance that resulted in a very time consuming
process that goes over every possible ID reference to find out if it needs to be updated.

If there are N of references to ID blocks and there are M ID blocks that needed to be remapped
it would take N*M checks. These checks are scattered around the place and memory.
Each reference would only be updated at most once, but most of the time no update is needed at all.

Idea: By grouping the changes together will reduce the number of checks resulting in improved performance.
This would only require N checks. Additional benefits is improved data locality as data is only loaded once
in the L2 cache.

It has be implemented for the resyncing process and UI editors.
On an Intel(R) Core(TM) i7-6700 CPU @ 3.40GHz 16Gig the resyncing process went
from 170 seconds to 145 seconds (during hotspot recording).

After this patch has been applied we could add similar approach
to references (references between data blocks) and functionality (tagged deletion).
In my understanding this could reduce the resyncing process to less than a second.
Opening the village production file between 10 and 20 seconds.

Flame graphs showing that UI remapping isn't visible anymore (WM_main_remap_editor_id_reference)

  • Master
  • This patch

Diff Detail

Repository
rB Blender
Branch
temp-T94185-id_remapping-experiment-a
Build Status
Buildable 19536
Build 19536: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Small fixes to crashes.
  • Moved IDRemapper up one level.
  • Moved remapping one level up.
  • Add multiple remapping to sync override.

I know this is just a draft. Just read over it and thought I'd leave a few code style comments. Do with that whatever you want.

source/blender/blenkernel/intern/lib_id_remapper.cc
36

Can just use add instead of add_as here, because no type conversion is necessary. Either way is fine though.

52

I think it should be possible to only do a single lookup in the map. Shouldn't matter much in practice, but thought I'd just mention it. (could use Map.lookup_ptr)

75

Doesn't matter too much, since this class currently isn't used from c++ directly.
It might be nice the take a FunctionRef as parameter instead of a separate function pointer and user data.

99

Should be possible to remove the _const suffix from the function name. Normal function overloading should automatically use the correct one.

119

missing newline

Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Dec 21 2021, 8:27 AM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Dec 21 2021, 8:34 AM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Dec 21 2021, 9:25 AM

@Jacques Lucke (JacquesLucke) thanks for the feedback.
I am happy to apply them.

Note that this patch is a research exeriment on time reduction during resyncing.
I am interested in discussing concepts :-)

Jeroen Bakker (jbakker) marked 2 inline comments as done.
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)
  • Add early exit when remapping multiple.
  • Fix incorrect logic performing way to many remaps.
  • Use add, moved wrap/unwrap outside C section.
source/blender/blenkernel/intern/lib_id_remapper.cc
99

I will move them outside the C section

Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Dec 21 2021, 9:56 AM
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)

I looked at libblock_remap_data but the code isn't that easy to read. So didn't add it to the scope of this patch.

Don't see anything wrong with the general idea behind that patch... Regrouping is indeed often a very efficient way to reduce those kind of complexity issues.

When direction is discussed and agreed upon we could add similar approach to references (references between data blocks) and functionality (tagged deletion).

Yes totally, this will likely give an even better performances improvement I think... And do not see any reason why this would not work in theory. Guess the only potential cans of worms will be our specific pre/post-processes here, not sure how hard those would be to convert safely.


Did not do any proper code review yet, below are just some quick notes found on first skim-reading the new code.

source/blender/blenkernel/BKE_lib_remap.h
163

Would rather use ID_REMAP_RESULT_ prefix here, to make it more clear what it is.

173

Usually prefer to clearly separate bitflags values and 'helper defines' in that kind of enum, and put the 'defines' at the end, after all the bitflags have been defined.

See e.g. the LIB_ID_CREATE_/LIB_ID_COPY_ ones in BKE_lib_id.h

181

No \brief, just redundant info. ;)

source/blender/blenkernel/intern/lib_override.c
1278 ↗(On Diff #46258)

Picky: Not a big fan of those extra indents in modern C, you can declare a new variable anywhere now, so they are not needed. And they do not really help with readability imho.

For code clarity and sanity, just set remapper to NULL at the end after freeing it.

source/blender/blenkernel/intern/lib_override.c
1278 ↗(On Diff #46258)

I totally agree with you.
Would rather move this to a separate function. I used the additional indent only to make sure it could be done.

Jeroen Bakker (jbakker) marked 3 inline comments as done.
  • Merge branch 'master' into temp-T94185-id_remapping-experiment-a
  • Changed enum values and names.
  • Extracted remap to separate function.
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Jan 19 2022, 9:48 AM
source/blender/editors/util/ed_util.c
448

I find strange that a user may be exposed to this message when running Blender.

Could we either:

  • Get rid of all ED_spacedata_id_remap_old
  • Make the print only bring in DEBUG mode
  • Remove the printf and add a TODO instead.
Jeroen Bakker (jbakker) planned changes to this revision.Jan 21 2022, 3:47 PM
Jeroen Bakker (jbakker) added inline comments.
source/blender/editors/util/ed_util.c
448

Yes this patch needs a cleanup. Was bit to quick to move it from experimental to review.
Would add a TODO as not all code can be converted to the new one.

Jeroen Bakker (jbakker) marked an inline comment as done.
  • Performance: Remap multiple items in UI
Jeroen Bakker (jbakker) marked an inline comment as done.Jan 24 2022, 12:12 PM
Jeroen Bakker (jbakker) added inline comments.
source/blender/blenkernel/intern/lib_id_remapper.cc
2

Missing license header

source/blender/windowmanager/intern/wm_init_exit.c
255 ↗(On Diff #46539)

ED_spacedata_id_remap_old is confusing.

  • Remove confusing name _old(

Besides knit-picky details below, LGTM.

source/blender/blenkernel/BKE_lib_remap.h
164

e.g.

198

id_ptr_ptr (and update name if changed oc)

198

if, not id, I guess?

201

would rather call it r_id_ptr ?
We use r_ prefix for values which are return values or can be modified by the function.

source/blender/blenkernel/intern/lib_id_remapper.cc
52

Would also assert that old_id and new_id are of the same type?

62

same remark regarding naming, would prefer r_id_ptr

119

Still missing ;)

source/blender/blenkernel/intern/lib_remap.c
518 ↗(On Diff #47372)

_ex is usually for an extended version of a function... This is more like a callback for each items of the remapper.

libblock_remap_iter_cb? libblock_remap_foreach_idpair_cb?...

source/blender/editors/space_outliner/space_outliner.cc
412–415 ↗(On Diff #47372)

Is this removal a bugfix? or done by mistake?

If bugfix, would rather have it committed separately first, using 'old' existing code.

425 ↗(On Diff #47372)

Would also rather see this fix committed separately.

Jeroen Bakker (jbakker) marked 10 inline comments as done.
  • Merge branch 'master' into temp-T94185-id-remapper-ui
  • Tweaks in naming (r_id_ptr and idpair_cb)
source/blender/editors/space_outliner/space_outliner.cc
412–415 ↗(On Diff #47372)

Was done deliberately old_id isn't available anymore and the new check isn't that useful anymore as most id types will continue. (ID_PC would be the only ID that would be filtered out ATM, and that that seems to be a bug :-)

if (!BKE_id_remapper_has_mapping_for(
FILTER_ID_SCE |
FILTER_ID_LI |
FILTER_ID_OB |
FILTER_ID_ME |
FILTER_ID_CU |
FILTER_ID_MB |
FILTER_ID_NT |
FILTER_ID_MA |
FILTER_ID_TE |
FILTER_ID_IM |
FILTER_ID_LT |
FILTER_ID_LA |
FILTER_ID_CA | 
FILTER_ID_KE |
FILTER_ID_WO |
FILTER_ID_SPK |
FILTER_ID_GR |
FILTER_ID_AR |
FILTER_ID_AC |
FILTER_ID_BR |
FILTER_ID_PA |
FILTER_ID_GD |
FILTER_ID_LS |
FILTER_ID_LP |
FILTER_ID_HA |
FILTER_ID_PT |
FILTER_ID_VO |
FILTER_ID_SIM |
FILTER_ID_SCR |
FILTER_ID_WM |
FILTER_ID_TXT |
FILTER_ID_VF |
FILTER_ID_SO |
FILTER_ID_CF |
FILTER_ID_PAL |
FILTER_ID_MC |
FILTER_ID_WS |
FILTER_ID_MSK |
) {
return false;
}
NOTE: LI, KE, SCR and WM are not supported as filter types. We could add a filter type for library.

So based on the additional complexity vs the very limited benefit I have decided to remove it.

425 ↗(On Diff #47372)

Check will do.

  • Merge branch 'master' into temp-T94185-id-remapper-ui
  • Fix assert logic.
This revision is now accepted and ready to land.Jan 25 2022, 2:44 PM
source/blender/blenkernel/intern/lib_id_remapper.cc
52

To use Map::lookup_ptr we should use std::optional<ID*> as value. It describes better what is supported and reduces lookup. Might look into this later.

This revision was automatically updated to reflect the committed changes.
Jeroen Bakker (jbakker) reopened this revision.EditedJan 25 2022, 3:38 PM

Tests are failing.

# Blender 3.1.0, Commit date: 2022-01-25 09:45, Hash 4010b17d474e

# backtrace
/home/jeroen/blender-git/build_linux/bin/blender(BLI_system_backtrace+0x37) [0xb2c2ff7]
/home/jeroen/blender-git/build_linux/bin/blender() [0x10e4a17]
/lib/x86_64-linux-gnu/libc.so.6(+0x46520) [0x7fea1c96c520]
/lib/x86_64-linux-gnu/libc.so.6(+0x19ff3d) [0x7fea1cac5f3d]
/lib/x86_64-linux-gnu/libc.so.6(+0x7ae68) [0x7fea1c9a0e68]
/lib/x86_64-linux-gnu/libc.so.6(+0x8c41a) [0x7fea1c9b241a]
/home/jeroen/blender-git/build_linux/bin/blender() [0x24b59b8]
/home/jeroen/blender-git/build_linux/bin/blender(CLG_logf+0x13a) [0x24b604a]
/home/jeroen/blender-git/build_linux/bin/blender(id_us_ensure_real+0x8f) [0x113830f]
/home/jeroen/blender-git/build_linux/bin/blender() [0x11383e9]
/home/jeroen/blender-git/build_linux/bin/blender(BKE_lib_query_foreachid_process+0x7f) [0x11413bf]
/home/jeroen/blender-git/build_linux/bin/blender(BKE_screen_foreach_id_screen_area+0x1c0) [0x11ecfe0]
/home/jeroen/blender-git/build_linux/bin/blender() [0x11ed2b3]
/home/jeroen/blender-git/build_linux/bin/blender() [0x11415c3]
/home/jeroen/blender-git/build_linux/bin/blender(BKE_library_foreach_ID_link+0x1d) [0x114193d]
/home/jeroen/blender-git/build_linux/bin/blender(BKE_main_id_refcount_recompute+0x122) [0x113a0a2]
/home/jeroen/blender-git/build_linux/bin/blender(BLO_library_link_end+0x9d) [0x15f8e8d]
/home/jeroen/blender-git/build_linux/bin/blender(BKE_blendfile_link+0x15b) [0x22f308b]
/home/jeroen/blender-git/build_linux/bin/blender() [0x15499f3]
/home/jeroen/blender-git/build_linux/bin/blender() [0x153ba5b]
/home/jeroen/blender-git/build_linux/bin/blender(WM_operator_call_py+0x6c) [0x153c79c]
/home/jeroen/blender-git/build_linux/bin/blender() [0x1add010]
/home/jeroen/blender-git/build_linux/bin/blender() [0x9db8fe4]
/home/jeroen/blender-git/build_linux/bin/blender(_PyObject_MakeTpCall+0x90) [0x9d7ad00]
/home/jeroen/blender-git/build_linux/bin/blender(_PyEval_EvalFrameDefault+0x6b09) [0x10de649]
/home/jeroen/blender-git/build_linux/bin/blender() [0x9e284c4]
/home/jeroen/blender-git/build_linux/bin/blender(_PyFunction_Vectorcall+0x9a) [0x9d7aa5a]
/home/jeroen/blender-git/build_linux/bin/blender(_PyObject_FastCallDictTstate+0xca) [0x9d7aefa]
/home/jeroen/blender-git/build_linux/bin/blender(_PyObject_Call_Prepend+0xe4) [0x9d7b144]
/home/jeroen/blender-git/build_linux/bin/blender() [0x9ddbe89]
/home/jeroen/blender-git/build_linux/bin/blender(_PyObject_MakeTpCall+0x90) [0x9d7ad00]
/home/jeroen/blender-git/build_linux/bin/blender(_PyEval_EvalFrameDefault+0x625c) [0x10ddd9c]
/home/jeroen/blender-git/build_linux/bin/blender() [0x10d6bdb]
/home/jeroen/blender-git/build_linux/bin/blender() [0x9d7d6d8]
/home/jeroen/blender-git/build_linux/bin/blender(_PyEval_EvalFrameDefault+0x59cc) [0x10dd50c]
/home/jeroen/blender-git/build_linux/bin/blender() [0x10d6bdb]
/home/jeroen/blender-git/build_linux/bin/blender(_PyEval_EvalFrameDefault+0x678a) [0x10de2ca]
/home/jeroen/blender-git/build_linux/bin/blender() [0x10d6bdb]
/home/jeroen/blender-git/build_linux/bin/blender(_PyEval_EvalFrameDefault+0x59cc) [0x10dd50c]
/home/jeroen/blender-git/build_linux/bin/blender() [0x9e284c4]
/home/jeroen/blender-git/build_linux/bin/blender(_PyEval_EvalCodeWithName+0x4e) [0x9e2880e]
/home/jeroen/blender-git/build_linux/bin/blender(PyEval_EvalCodeEx+0x3e) [0x9e2885e]
/home/jeroen/blender-git/build_linux/bin/blender(PyEval_EvalCode+0x1b) [0x9e2888b]
/home/jeroen/blender-git/build_linux/bin/blender() [0x9e63205]
/home/jeroen/blender-git/build_linux/bin/blender(PyRun_FileExFlags+0xad) [0x9e6533d]
/home/jeroen/blender-git/build_linux/bin/blender() [0x1ac16cf]
/home/jeroen/blender-git/build_linux/bin/blender() [0x10e2a18]
/home/jeroen/blender-git/build_linux/bin/blender(BLI_args_parse+0xd1) [0xb1ed4b1]
/home/jeroen/blender-git/build_linux/bin/blender(main+0x2e7) [0xfd3ff7]
/lib/x86_64-linux-gnu/libc.so.6(+0x2dfd0) [0x7fea1c953fd0]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x7d) [0x7fea1c95407d]
/home/jeroen/blender-git/build_linux/bin/blender(_start+0x25) [0x10e0e95]

# Python backtrace
  File "/home/jeroen/blender-git/build_linux/bin/3.1/scripts/modules/bpy/ops.py", line 132 in __call__
  File "/home/jeroen/blender-git/blender/tests/python/bl_blendfile_library_overrides.py", line 53 in test_link_and_override_property
  File "/home/jeroen/blender-git/blender/tests/python/bl_blendfile_utils.py", line 35 in run_all_tests
  File "/home/jeroen/blender-git/blender/tests/python/bl_blendfile_library_overrides.py", line 215 in main
  File "/home/jeroen/blender-git/blender/tests/python/bl_blendfile_library_overrides.py", line 222 in <module>
This revision is now accepted and ready to land.Jan 25 2022, 3:38 PM
Jeroen Bakker (jbakker) planned changes to this revision.Jan 25 2022, 3:38 PM
This revision was not accepted when it landed; it landed in state Changes Planned.Jan 26 2022, 11:30 AM
This revision was automatically updated to reflect the committed changes.