Skip to content

Conversation

@timholy
Copy link
Member

@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
Copy link
Member 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
Member

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
Member

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

@timholy
Copy link
Member 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
Member 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
Member 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.

3 participants