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

TOMLCache #68

Closed
wants to merge 2 commits into from
Closed

TOMLCache #68

wants to merge 2 commits into from

Conversation

timholy
Copy link
Owner

@timholy timholy commented Sep 16, 2020

This started out as a performance optimization, but I found if you load Revise first and then run the tests they fail, I think because of the new TOMLCache APIs.

`using Images` spends about 60ms reparsing Manifest.toml multiple times.
If possible, we might as well avoid that.
@timholy timholy requested a review from KristofferC September 16, 2020 22:44
timholy added a commit to timholy/Revise.jl that referenced this pull request Sep 17, 2020
@timholy
Copy link
Owner Author

timholy commented Sep 17, 2020

Test failure is JuliaLang/julia#37586 (comment)

Companion Revise PR: timholy/Revise.jl#533

But there's one thing that bothers me about this: without Revise to invalidate the cache when the manifest file changes, won't this give wrong answers? It really seems like we should have Base.loading do the invalidation. We could have Base do something like this:

const external_tomlcaches = RefValue{Union{Nothing, Base.TOMLCache}}[]

...
# inside the code that changes the active Manifest.toml
for extcache in external_tomlcaches
    extcache[] = nothing
end

But even easier, is there any reason not to essentially move the implementation here to Base?

@KristofferC
Copy link
Collaborator

won't this give wrong answers?

Yes, it will so I don't think you can have a global cache like this now. You would need these functions to accept a cache, and then you send that from Revise and also invalidate it from Revise.

But even easier, is there any reason not to essentially move the implementation here to Base?

Not really. I just thought it was hard to do it robustly, but I didn't try for very long.

@KristofferC
Copy link
Collaborator

Regarding JuliaLang/julia#37586 (comment) did you try JuliaLang/julia#37608?

@timholy
Copy link
Owner Author

timholy commented Sep 17, 2020

Regarding JuliaLang/julia#37586 (comment) did you try JuliaLang/julia#37608?

I'd missed that, will test.

@timholy
Copy link
Owner Author

timholy commented Sep 17, 2020

We probably don't want to merge this as-is, so I'm marking it as a draft.

@timholy timholy marked this pull request as draft September 17, 2020 10:56
@timholy timholy mentioned this pull request Oct 12, 2020
@timholy timholy closed this in #71 Oct 12, 2020
@timholy timholy deleted the teh/tomlcache branch October 18, 2020 08:13
@timholy
Copy link
Owner Author

timholy commented Oct 18, 2020

Obviated by JuliaLang/julia#37906

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.

2 participants