-
Notifications
You must be signed in to change notification settings - Fork 39
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
internal: speed up FileInfoIdx
-from-path lookup
#859
Conversation
…act on compilation speed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome, and thank you for working on this.
I've left a few requests regarding code style, data placement, and documentation. In addition, please update the PR message and title according to the contribution guidelines. The guidelines contain examples of how the title and message should look like, and for inspiration, you can also look at PRs merged previously.
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. There are few things I missed during my earlier review that still need to be changed, but apart from those and the PR message/title, this is good to.
For the PR title, make sure it reads like a changelog entry (it should also not include the fixed issue) and is 50 characters in length (a few more are generally okay).
The "summary" section should provide a short overview of what changed and why it changed. The "details" section should provide details about the implementation and steps taken. In addition, each line in the message body must be no longer than 72 characters.
As it stands, most of what you have placed in the "summary" section should be part of the "details" section.
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
Co-authored-by: zerbina <100542850+zerbina@users.noreply.github.com>
The I'll quickly make a separate PR with a fix, after which you can merge |
Okay, the fix is merged. After merging the |
now all passed, am not native English speaker, feel free to modify the title and description. |
FileInfoIdx
-from-path lookup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated both the PR message and title. Thank you seeing this through. Nice work! IC is a lot faster now.
I did remove the mention of #546, however, as the caching doesn't affect the --filenames=canonical
mode more than it affects the other modes (canonicalImportAux
doesn't use fileInfoIdx
).
Thank you for review my PR and make a better description. glad to hear it getting faster, nice! |
/merge |
Merge requested by: @zerbina Contents after the first section break of the PR description has been removed and preserved below: |
That's a very nice performance improvement, thanks @bung87 |
Summary
Cache the result of file-info-from-absolute-path lookups, avoiding
unnecessary filesystem IO incurred by expanding file paths. This
significantly increases the speed of the incremental compilation mode
(which performs a lot of those lookups), with compile time reductions
of up to 90% (measured with the
tstdlib_import_changed.nim
test).Details
Add the
rawPathToIndexTbl
table toMsgFormat
. It associates theunprocessed absolute file paths passed to
fileInfoIdx
with theirresolved-to
FileInfoIdx
, preventing the repeated expansion of thesame file paths.
In addition, the line overrides in the
assert
andjsAssert
templates are changed to use the no-argument form (
{.line.}
). Thelatter is semantically equivalent to the previous
{.line: loc.}
,while not performing an unnecessary
FileInfoIdx
-> filename ->FileInfoIdx
round-trip.