BlenRead: Add GHash-based search for already read linked IDs.
ClosedPublic
Actions

Authored by Bastien Montagne (mont29) on Jun 30 2021, 3:52 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Campbell Barton (campbellbarton)

Commits

rBdb4fe8e3223b: BlenRead: Add GHash-based search for already read linked IDs.

Summary

Ths commit adds a new IDNameLibMap to Main, used during file reading
to quickly find already read linked IDs.

Without that, search would use string-based search over list of linked
data, which becomes extremely slow and inneficient in cases where a lot
of IDs are linked from a same library. See also T89194: Opening even remotely complex scenes with many objects takes much longer than before.

Extrem-usecase reported in T89194 is now about 4 times faster in linked
data reading (about 2 times faster for the whole .blend file loading).

More normal cases (like Sprites studio production files) have barely
measurable speed improvements, a few percents at best.

NOTE: main_idmap API was extended to support insertion and removal of IDs from the mapping, avoids having to re-create the whole thing several time during libraries expansion in readcode.

Diff Detail

Repository: rB Blender

Event Timeline

Bastien Montagne (mont29) requested review of this revision.Jun 30 2021, 3:52 PM

Bastien Montagne (mont29) created this revision.

Harbormaster completed remote builds in B15518: Diff 38967.Jun 30 2021, 3:52 PM

I'm having a hard time verifying if this code is correct, and if datablocks are being added to or removed from this idmap in all cases that are required. As far as I can tell it's ok.

I would have tried to wrap BLI_addtail, id_sort_by_name and BKE_main_idmap_insert_id in a single function that is used whenever a new datablock is added. But I'm not sure how difficult that is.

source/blender/blenloader/intern/readfile.c
480–484	I would destroy this before joining, not after. Not that it makes a big difference now, but imagine `add_main_to_main` uses this map in the future.

This revision is now accepted and ready to land.Jul 5 2021, 6:11 PM

Minor update to reflect state of final commit.

Harbormaster completed remote builds in B16063: Diff 40006.Jul 27 2021, 2:53 PM

Thanks for the review!

In D11757#304058, @Brecht Van Lommel (brecht) wrote:

I'm having a hard time verifying if this code is correct, and if datablocks are being added to or removed from this idmap in all cases that are required. As far as I can tell it's ok.

Yes same... Did open a lot of files, ran the tests, tried to link data, all seems to work, so fingers crossed.

I would have tried to wrap BLI_addtail, id_sort_by_name and BKE_main_idmap_insert_id in a single function that is used whenever a new datablock is added. But I'm not sure how difficult that is.

Thing is, the only place where that would have helped is in create_placeholder. read_libblock iss too much intricate these days with undo code, to allow such nice factorization.

Closed by commit rBdb4fe8e3223b: BlenRead: Add GHash-based search for already read linked IDs. (authored by Bastien Montagne (mont29)). · Explain WhyJul 27 2021, 3:00 PM

This revision was automatically updated to reflect the committed changes.

Bastien Montagne (mont29) added a commit: rBdb4fe8e3223b: BlenRead: Add GHash-based search for already read linked IDs..