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

Change outlineItem ID assignment process for major optimization #827

Merged
merged 2 commits into from Apr 8, 2021

Conversation

emgineering
Copy link
Contributor

This code uses an O(1) process to assign IDs (keep track of the largest ID that has been assigned, and make sure to assign an ID larger than that), compared to the existing O(n^2) process (get a list of all existing IDs, and check every candidate ID starting from 1 until finding one not in the list).

My testing has shown that currently, of all the time spent reading from file, parsing, and setting data, 99.7% of it is spent in the findUniqueID() method. Making this change has led to a speedup from ~18 sec. to ~2 sec on loading my manuscript, while still guaranteeing that any (new) objects have unique IDs.

@TheJackiMonster
Copy link
Collaborator

Wow, thanks for fixing that. Existing IDs in old projects won't be replaced using these changes, right? So it's safe to merge, I assume.

Can you squash the commits to one for merging?

@TheJackiMonster TheJackiMonster added this to the 0.12.0 milestone Apr 8, 2021
@TheJackiMonster
Copy link
Collaborator

Does your PR make #777 redundant? I haven't really looked into the details of the ID distribution to see that directly. ^^'

@emgineering
Copy link
Contributor Author

emgineering commented Apr 8, 2021

Squashed.

Existing IDs in old projects won't be replaced using these changes, right?

Should be fine. Previously assigned IDs are read from the file at load time. (In general, calling "setData" with an ID forces the ID assignment, and informs the distributor to not give that ID out again when automatically assigning.)

Does your PR make #777 redundant? I haven't really looked into the details of the ID distribution to see that directly. ^^'

Took a look - as far as I can tell, it's dealing with a separate issue, and would work fine with my changes. (It ultimately calls "setData", which this code has handled.) It does use the slow method for checking for valid IDs, though; all the code there could probably be replaced with item.getUniqueID(recursive=true) to take advantage of the speedups and be more consistent with the rest of the project.

@TheJackiMonster
Copy link
Collaborator

Thanks for the insight. I will refer to this PR then in #777. ^^

@TheJackiMonster TheJackiMonster merged commit cdb5e3d into olivierkes:develop Apr 8, 2021
TheJackiMonster added a commit that referenced this pull request Apr 10, 2021
Signed-off-by: TheJackiMonster <thejackimonster@gmail.com>
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

2 participants