More extensive record-keeping during module loading/compilation #23898

timholy · 2017-09-27T08:53:38Z

Currently Revise has to try to parse source code to figure out what files a package depends on and which module a file should be evaluated into. This is fragile. The two commits here should allow Revise to work sensibly on Julia 1.0:

callbacks for include so that other packages can log these dependencies (important for __precompile__(false) packages)
changes to the precompile cache file to also store the evaluating module (useful for __precompile__(true) packages)

EDIT: now it also saves the raw source text (as a string) to the cache file, addressing #23448.

This allows one to obtain the file dependencies of a non-precompiled package.

timholy · 2017-10-03T05:04:38Z

@vtjnash, if possible it would be great to have you look this over, since this makes a number of changes to the format of the cache files.

vchuravy

I think it is time that we document the cachefile format.
IIUC this now also stores the source text of a dependent files in the cache file so that it is easier accessible for down-stream consumers?

vchuravy · 2017-10-03T12:25:58Z

test/compile.jl

-        @test map(x -> x[1],  sort(deps)) == [Foo_file, joinpath(dir, "bar.jl"), joinpath(dir, "foo.jl")]
+        @test map(x -> x[1],  sort(discard_module.(deps))) == [Foo_file, joinpath(dir, "bar.jl"), joinpath(dir, "foo.jl")]
+        srctxt = Base.read_dependency_src(cachefile, Foo_file)
+        @test !isempty(srctxt) && srctxt == read(Foo_file, String)


Can we add a test for a manually included dependency. If I understand one could end up parsing the list of dependent files and end up with an ("__external__", "external.so", mtime) and `Base.read_dependency_src(cachefile, "external.so") should fail.

Right, but IMO that's fine. This is an internal method and consumers can make sure they don't try to read "__external__" dependencies. (Reading the src text is not something that's done unless it is requested.) Revise doesn't intend to track external dependencies, because it's hard to know what you're supposed to do about them.

But if you know of packages that are using this feature, I would be happy to go look at how they're using external dependencies; that could end up changing my mind.

Everything that would lead to a stale cachefile should probably prompt Revise to to reload that file :).
In my use cases I have stateful information from that external dependency (if it exists or not, API version, ...)

Would love a link to your use cases, if they are in public repos.

It would be fine to trigger revise(module) when an external dependency updates, but it seems like it can't be robust: what if the external dependency is a single file in another package? How is Revise supposed to know whether it has to revise that package first? (In some cases it wouldn't but in others it would be necessary.)

In either case, I don't think Revise will need access to the source text of external dependencies. If, as you say, it's an .so file, Revise can't do anything with it anyway. In fact it occurs to me that we probably shouldn't store external dependencies in the src-text cache; it would be crazy to cache, say, libkde.so in a *.ji file 😄.

I will change the format so that each source-text file has its name at the beginning, so that one doesn't have to "count" from the list of dependencies while skipping over #__external__ dependencies.

I was wondering if that might happen ;) yeah that would be crazy.

vchuravy · 2017-10-03T12:30:34Z

base/loading.jl

@@ -230,7 +234,7 @@ This is only needed if your module depends on a file that is not used via `inclu
 no effect outside of compilation.
 """
 function include_dependency(path::AbstractString)
-    _include_dependency(path)
+    _include_dependency("__external__", path)


Currently __external__ is a perfectly valid module name, albeit odd.

Solutions I see:

make __external__ a reserved module name.

add a flag to signify a external file.

How about I change it to #__external__? That seems pretty much impossible unless someone goes to heroic effort to generate it, @eval using $(Symbol("#__external__")).

That sounds good even though I would love a verification step when we write out the module, but maybe that is easier to add to Revise.

This ensures that it's possible to re-evaluate modified code on a file-by-file basis (as with Revise)

timholy · 2017-10-04T09:40:06Z

Barring objections, I'm planning to merge this by the end of the week.

vchuravy

LGTM.

vtjnash · 2017-10-04T18:52:25Z

src/dump.c

+                ios_write(&f, jl_string_data(dep), slen);
+                posfile = ios_pos(&f);
+                write_uint64(&f, 0);   // placeholder for length of this file in bytes
+                ios_file(&srctext, jl_string_data(dep), 1, 0, 0, 0);


Needs error handling. Alternatively, might be nice to store the contents as they are used. Also might be nice to use some standard format. For example, LLVM provides llvm::writeArchive (http://llvm.org/doxygen/ArchiveWriter_8h_source.html#l00041) for copying a list of files (paths and/or buffers) into the ar format (although LLVM may need a small patch to disable throwing an error when the symbol table cannot be created when it is not needed)

I just added some simple error handling, hopefully it's what you had in mind.

The LLVM interaction seems a little scarier, since I'm less familiar with it. Dumb question: how do you even call out to LLVM from a *.c file?

The thing I like about storing this in the *.ji file is that they are guaranteed to be validated/invalidated together. Otherwise, I agree with you that a standard format has advantages.

Closes #23448

stevengj · 2018-06-25T15:56:11Z

I was looking at whether I should call include_callbacks in NBInclude.jl, but it looks like Revise.jl expects the callback parameter to be a Julia file. Might be nice to pass the include function (e.g. nbinclude) to the callback. Or maybe a parse function? Not sure what would be needed here to handle Julia code that is stored in structured formats like Jupyter notebooks (JSON).

timholy requested a review from vtjnash September 27, 2017 08:54

timholy mentioned this pull request Sep 27, 2017

Packages with fancy include mechanisms timholy/Revise.jl#40

Closed

vchuravy approved these changes Sep 27, 2017

View reviewed changes

timholy changed the title ~~More extensive record-keeping during module loading~~ More extensive record-keeping during module loading/compilation Sep 27, 2017

timholy mentioned this pull request Sep 28, 2017

Eliminate need to parse include paths timholy/Revise.jl#49

Merged

KristofferC mentioned this pull request Oct 2, 2017

use literals for includes JuliaDocs/Documenter.jl#572

Merged

Add a hook for callbacks when files are included.

5ed9551

This allows one to obtain the file dependencies of a non-precompiled package.

timholy force-pushed the teh/include_hook branch from a23e416 to ec870cd Compare October 3, 2017 04:38

timholy mentioned this pull request Oct 3, 2017

Wishlist: caching source code #23448

Closed

timholy force-pushed the teh/include_hook branch 3 times, most recently from 6b2306d to 2163e3d Compare October 3, 2017 04:59

timholy force-pushed the teh/include_hook branch 2 times, most recently from 9a1d74c to 15569d9 Compare October 3, 2017 11:55

vchuravy requested changes Oct 3, 2017

View reviewed changes

Store the evaluating module in precompile cache

a053453

This ensures that it's possible to re-evaluate modified code on a file-by-file basis (as with Revise)

timholy force-pushed the teh/include_hook branch from 15569d9 to 7476cbc Compare October 3, 2017 15:25

vchuravy approved these changes Oct 4, 2017

View reviewed changes

vtjnash reviewed Oct 4, 2017

View reviewed changes

Store a copy of the source code text in cachefiles

ef28c11

Closes #23448

timholy force-pushed the teh/include_hook branch from 7476cbc to ef28c11 Compare October 4, 2017 19:38

timholy merged commit b9a0d1d into master Oct 7, 2017

timholy deleted the teh/include_hook branch October 7, 2017 10:43

timholy mentioned this pull request Apr 11, 2020

Support user-specified callbacks timholy/Revise.jl#443

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More extensive record-keeping during module loading/compilation #23898

More extensive record-keeping during module loading/compilation #23898

timholy commented Sep 27, 2017 •

edited

Loading

timholy commented Oct 3, 2017

vchuravy left a comment

vchuravy Oct 3, 2017

timholy Oct 3, 2017 •

edited

Loading

vchuravy Oct 3, 2017

timholy Oct 3, 2017

vchuravy Oct 3, 2017

vchuravy Oct 3, 2017

timholy Oct 3, 2017

vchuravy Oct 3, 2017

timholy commented Oct 4, 2017

vchuravy left a comment

vtjnash Oct 4, 2017

timholy Oct 4, 2017

stevengj commented Jun 25, 2018

More extensive record-keeping during module loading/compilation #23898

More extensive record-keeping during module loading/compilation #23898

Conversation

timholy commented Sep 27, 2017 • edited Loading

timholy commented Oct 3, 2017

vchuravy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy commented Oct 4, 2017

vchuravy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Jun 25, 2018

timholy commented Sep 27, 2017 •

edited

Loading

timholy Oct 3, 2017 •

edited

Loading