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

avoid digesting the rsc classpath on each iteration #7837

Conversation

cosmicexplorer
Copy link
Contributor

Problem

We currently convert the rsc tool's classpath into PathGlobsAndRoot and call self.context._scheduler.capture_snapshots() on every single rsc invocation (unless --rsc-native-image is provided). We don't need to digest these files every time -- they should remain the same across all invocations.

Solution

  • Create the memoized self._rsc_classpath_snapshot to avoid snapshotting the rsc classpath more than once.

Result

There may be a slight performance boost to the rsc compile task.

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks!

It looks like #7838 represents the only other source of the input_files parameter here. Should be able to remove it in whichever PR lands first.

PathGlobs(tuple(cp_rel)),
text_type(get_buildroot()))
cp_snapshot, = self.context._scheduler.capture_snapshots((cp_globs,))
# TODO: A Snapshot can have `.files` and `.dirs` in arbitrary order, which doesn't allow
Copy link
Member

Choose a reason for hiding this comment

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

The files and dirs in a Snapshot are sorted post #7241.

# using a `Digest`ed classpath requires separately passing along the original (relativized)
# classpath paths. Individual `ClasspathEntry`s can be created, but those only allow a single
# path per digest, which isn't possible when digesting multiple paths at once, as we do here.
return (cp_rel, cp_snapshot)
Copy link
Member

Choose a reason for hiding this comment

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

Consider using a ClasspathEntry for this?

@illicitonion
Copy link
Contributor

I picked this up and finished it off in another PR :) Thanks for starting it off!

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