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

Speed cycle checking. #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

robinwatts
Copy link
Contributor

When run on a codebase with a large dependency tree the cycle checking can take an enormous amount of time.

In my tests, attempting to make JOM work with ghostscript's makefiles, it can basically take forever (I gave up after 10 minutes).

There is a simple fix for this, implemented here. Once we have cycle checked downwards from a given node, we know that whenever we reach that node again, we need not bother to recheck it as the results will be no different.

Hence whenever we successfully return from the recursive call, set a flag meaning "NoCyclesRootedAtThisNode". Whenever we step into a node, if that flag is set, there is no need to recurse at all.

This is sufficient to make the cycle checking instant for the Ghostscript makefiles.

This does mean that we are left with a 'dirty' tree, in that the NoCyclesRootedAtThisNode flags are left set. I don't think this is an issue, (does Parser:apply ever get called more than once? And if it does, might the tree have changed between calls?) but just in case, I have introduced another pass where we run through the tree clearing the nodes.

When run on a codebase with a large dependency tree
the cycle checking can take an enormous amount of time.

In my tests, attempting to make JOM work with ghostscript's
makefiles, it can basically take forever (I gave up after
10 minutes).

There is a simple fix for this, implemented here. Once we have
cycle checked downwards from a given node, we know that whenever
we reach that node again, we need not bother to recheck it as
the results will be no different.

Hence whenever we successfully return from the recursive call,
set a flag meaning "NoCyclesRootedAtThisNode". Whenever we
step into a node, if that flag is set, there is no need to
recurse at all.

This is sufficient to make the cycle checking instant for the
Ghostscript makefiles.

This does mean that we are left with a 'dirty' tree, in that
the NoCyclesRootedAtThisNode flags are left set. I don't think
this is an issue, (does Parser:apply ever get called more than
once? And if it does, might the tree have changed between calls?)
but just in case, I have introduced another pass where we run
through the tree clearing the nodes.
There is no point in repeatedly traversing downwards from a
node once we've constructed the rules for it.

If a node has rules, just exit.

This vastly speed the time spent on complicated Makefiles like
Ghostscript.
@jobor
Copy link
Contributor

jobor commented Aug 8, 2023

Thanks for your contribution!
I've added these patches for review here:
https://codereview.qt-project.org/c/qt-labs/jom/+/495283
https://codereview.qt-project.org/c/qt-labs/jom/+/495282

You may close this merge request.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants