Skip to content

Conversation

@vtjnash
Copy link
Member

@vtjnash vtjnash commented Dec 11, 2022

These places in the code can either be more efficient O(1) or more correct using something more similar to the published SCC algorithm by Tarjan for strongly connected components.

These places in the code can either be more efficient O(1) or more
correct using something more similar to the published SCC algorithm by
Tarjan for strongly connected components.
static jl_array_t *jl_verify_graph(jl_array_t *edges, htable_t *visited)
{
arraylist_t stack;
arraylist_new(&stack, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an arraylist_reserve?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have no guess for the amount to reserve, so this is already optimal

int found = (char*)ptrhash_get(visited, (void*)caller) - (char*)HT_NOTFOUND;
if (found == 0)
return 1; // valid
return 1; // NOTFOUND == valid
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use an enum for these?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not easily, since we also end up doing some arithmetic with them

}

// Implements Tarjan's SCC (strongly connected components) algorithm, simplified to remove the count variable
static int jl_add_to_ee(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if this could return the SCCs as merged modules instead of committing them to the JIT, since that provides an extension point for any future work operating on the SCCs of a particular compile.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is true, it would give cleaner separation of responsibilities to only return the vector of lists of SCCs in order. But also more annoying to preserve that list. Ideally we would not merge them at all, but OrcJIT deleted the API for passing in SCC groups just as we were about ready to use it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can address this in a follow-up, to consider it in isolation.

@vtjnash vtjnash merged commit b03439c into master Dec 12, 2022
@vtjnash vtjnash deleted the jn/scc branch December 12, 2022 21:15
vtjnash added a commit that referenced this pull request Feb 6, 2023
These places in the code can either be more efficient O(1) or more
correct using something more similar to the published SCC algorithm by
Tarjan for strongly connected components.

(cherry picked from commit b03439c)
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.

3 participants