Skip to content

Fix quadratic-time ModuleNode.sort_types_by_inheritance#5139

Merged
scoder merged 4 commits intocython:masterfrom
swolchok:fix-quadratic-sort-types-by-inheritance
Nov 30, 2022
Merged

Fix quadratic-time ModuleNode.sort_types_by_inheritance#5139
scoder merged 4 commits intocython:masterfrom
swolchok:fix-quadratic-sort-types-by-inheritance

Conversation

@swolchok
Copy link
Contributor

At Meta, we have some large .pyx files generated by https://github.com/facebook/fbthrift . I found through profiling that ModuleNode.sort_types_by_inheritance was taking about 10% of the time for Cython to process our largest file, and through inspection that it took quadratic time. This PR changes sort_types_by_inheritance to usesorted to avoid quadratic time, with the result that sort_type_by_inheritance take a small fraction of a percent on the same large workload after this change.

This function seems to be covered by the cdef_class_order and ooo_base_classes tests; they failed during development and now pass.

@swolchok
Copy link
Contributor Author

This needs further work; will update when it's ready.

@da-woods
Copy link
Contributor

The failing tests are probably mostly an unrelated issue to do with a new version of line_profiler. So they may not be to do with this PR. I'll rerun the tests on this PR when that's fixed.

@swolchok
Copy link
Contributor Author

The failing tests are probably mostly an unrelated issue to do with a new version of line_profiler. So they may not be to do with this PR. I'll rerun the tests on this PR when that's fixed.

thanks! I'm seeing failures on the internal version of this as well, though; I'll have to debug those and add more tests to Cython.

@swolchok
Copy link
Contributor Author

The problem is the sort comparator I defined isn't a total ordering. I'll have to implement a proper topological sort.

It should take O(n log n) time now.
@swolchok swolchok force-pushed the fix-quadratic-sort-types-by-inheritance branch from b825a21 to d6b2900 Compare November 18, 2022 20:09
Co-authored-by: scoder <stefan_ml@behnel.de>
@da-woods
Copy link
Contributor

Closing and reopening to rerun CI

@da-woods da-woods closed this Nov 26, 2022
@da-woods da-woods reopened this Nov 26, 2022
@scoder
Copy link
Contributor

scoder commented Nov 29, 2022

I simplified the implementation. @swolchok, could you please validate that this doesn't lead to a performance regression on your side again?

@scoder scoder added this to the 3.0 milestone Nov 30, 2022
@scoder
Copy link
Contributor

scoder commented Nov 30, 2022

I'll merge this since it's a major improvement. We can have further PRs if we need more.

Thanks!

@scoder scoder merged commit 1acc5b1 into cython:master Nov 30, 2022
@swolchok swolchok deleted the fix-quadratic-sort-types-by-inheritance branch January 7, 2025 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants