Remove reflection perf #890
Conversation
… instead of per hidden reflection
| * Remove the specified reflections from the project. | ||
| */ | ||
| static removeReflections(project: ProjectReflection, reflections: Reflection[]) { | ||
| const deletedIds = new Array<number>(); |
There was a problem hiding this comment.
| const deletedIds = new Array<number>(); | |
| const deletedIds: number[] = []; |
| reflection.traverse((child) => CommentPlugin.removeReflection(project, child)); | ||
| private static removeReflection(project: ProjectReflection, reflection: Reflection, deletedIds: number[]) { | ||
| // keep track of the reflections that have been deleted | ||
| deletedIds.push(reflection.id); |
There was a problem hiding this comment.
I'd move this to after the delete just to make sure the delete actually took place.
|
This looks like a pretty solid improvement. I just tried this out with our project and it reduced our generation time by ~50% given the amount of hidden declarations we have. I suspect many large projects will benefit from this. Made some minor coding suggestions above. |
|
Thanks for reviewing @jonchardy! |
|
Thanks everyone! |
|
I just noticed one of the linked plugins in the readme uses CommentPlugin.removeReflection. https://github.com/christopherthielen/typedoc-plugin-external-module-name/blob/8152a66784227636c6a195923acd326c605d31dc/plugin.ts#L132 I think this change will break that. Should we consider keeping the API the same, or should plugin authors avoid using internal methods that could change? |
|
Ultimately it would be best if the implementation was moved out of the CommentPlugin if its meant for general usage. I think I saw some other PRs relating to trying to move that to context (and possibly raising notifications) which may be worth considering at some point. As to this issue, I guess one simple option would be to check for the deletedIds being provided and if not (i.e. its null/undefined) then perform the loop it used to. |
|
Oh yea. #632. @aciccarello @shlomiassaf Thoughts? I think moving it out of a plugin makes sense, and I also think we should have a second method so we can keep the performance improvements in place when doing a mass removal. |
|
For now though if you want to avoid breaking that plugin then I think just re-inserting the loop when the deletedIds is null is the right thing to do. I can try to create another PR with that later or tomorrow unless you want to do it. |
|
👍 I think both the compatibility fix and moving the method out of the plugin makes sense. |
There's a performance bug in the removeReflection because the symbolMapping is enumerated for each call. I have a large library where there are many types and members marked as @hidden. The CommentsPlugIn stores those in the hidden array and then later invokes the removeReflection for each. Each time the method is invoked it iterates the entire symbolMapping of the project which in my case started with around 90k items. So there were about 35k hidden items which including descendants resulted in about 300k calls to removeReflection.
This change breaks that up so that it invokes the removeReflection for each hidden item as it did except it does not touch the symbolMapping. It just tracks the id's of the items processed. Then after all the hidden items have been processed, it can just enumerate that once and use the id's of the hidden reflections and their descendants to determine which need to be deleted.