split SourceInfo out of LambdaInfo #18413

vtjnash · 2016-09-08T17:59:42Z

A number of recent changes to the system subtly broke the definition of LambdaInfo (most recently culminating in #18191), this aims to rectify that shift in identity by revising the way this data is passed around the system. And I expect this to be useful for fix #265, since the current issue I'm running into there is the possibility the system will try to duplicate an existing LambdaInfo (for example to re-infer it:

julia/base/inference.jl

Lines 1601 to 1621 in 424b3c5

    
           if inferred && code.inferred && linfo !== code 
        
               # This case occurs when the IR for a function has been deleted. 
        
               # `code` will be a newly-created LambdaInfo, and we need to copy its 
        
               # contents to the existing one to copy the info to the method cache. 
        
               linfo.inInference = true 
        
               linfo.code = code.code 
        
               linfo.slotnames = code.slotnames 
        
               linfo.slottypes = code.slottypes 
        
               linfo.slotflags = code.slotflags 
        
               linfo.ssavaluetypes = code.ssavaluetypes 
        
               linfo.pure = code.pure 
        
               linfo.inlineable = code.inlineable 
        
               linfo.propagate_inbounds = code.propagate_inbounds 
        
               ccall(:jl_set_lambda_rettype, Void, (Any, Any), linfo, code.rettype) 
        
               if code.jlcall_api == 2 
        
                   linfo.constval = code.constval 
        
                   linfo.jlcall_api = 2 
        
               end 
        
               linfo.inferred = true 
        
               linfo.inInference = false 
        
           end

) which made it difficult to track them and somewhat impossible to accurately invalidate them.

Major changes:

several functions no longer have "update their cache" as a goal. instead they return a value (they do still transparently maintain a cache, but that's now an implementation detail). most significantly, these include: jl_type_infer / typeinf_ext, jl_compile_for_dispatch / jl_compile_linfo, and jl_generate_fptr
type-inference now returns a SourceInfo tree (which may or may not be cached in the LambdaInfo that was passed in), allowing better (earlier) code deletion in some cases and easier thread-safety (less race-y to look at the code field)
the code field of LambdaInfo is gone, replaced by inferred and which is usually null. This fixes two of the lock priority inversion bugs, as now no Julia code needs to get run to create a LambdaInfo. And it should make it easier to delete the code (as that's a return to the default state rather than the presence of a third rare state), so that's done more aggressively now.
jlcall_api has been incremented by 1 to make it thread-safe to read without needing a lock or atomics ops in the fast-path of jl_call_method_internal (except jlcall_api = 2, which remains unchanged)
printing of LambdaInfo (e.g. from expand, code_lowered, code_typed, etc.) is corrected / improved / fixed, since it is no longer ambiguous whether to print the source or the signature

JeffBezanson · 2016-09-09T01:55:26Z

src/toplevel.c

    jl_methtable_t *mt;
    jl_sym_t *name;
    jl_method_t *m = NULL;
-    JL_GC_PUSH2(&f, &m);
+    jl_tupletype_t *argtype = jl_apply_tuple_type(atypes);


Why the change to make the tuple type here instead of in the front end? The method signature needs to be a type in any case, and in the future could be other kinds of types --- soon to be UnionAll types for methods with static parameters, and possibly later any type that's a subtype of Tuple.

The argument names and isva are directly mapped from the atypes svec. The translation to a tuple type in some cases is not information preserving (in particular with Vararg), so it is not an equivalent (or at least reversible) representation of the source information. In those cases this function becomes unable to form the map from the input argument list to the function argument number that stores contains value. I added a test for this case.

Bug acknowledged (and I'm very glad to get rid of the awful jl_is_rest_arg), but I don't think this is necessary to fix it. We could instead call jl_is_va_tuple on the tuple type. The natural interface for adding a method is to specify a type and a function body. In fact, any code that tries to manually take apart types will increasingly be wrong or buggy. For example Union{Tuple{A,B}, Tuple{C,D}} could be the type of a method, and you'll need to ask the type system to do something non-trivial to determine whether it is varargs, what the type of the nth argument is, etc.

isvararg != isvatuple. one such counter example is NTuple

Ok that's true, but long term, this needs to be a type.

Also: do we want f(x::T) to be varargs based on whether T evaluates to Vararg{...}? That seems like a misfeature to me. And since as you point out Tuple{Int,Int} and Tuple{Vararg{Int,2}} are equal types, the presence of Vararg is not a good basis for identifying varargs functions. It seems like this needs to be syntax-based and passed as a separate flag, since it's not really a property of the signature but of how argument names are bound to argument values. I suppose we can deal with that later though.

JeffBezanson · 2016-09-09T02:21:30Z

We should think about the names of the types a bit. LambdaInfo was always a bit jargony, but kind of made sense since it basically corresponded to a single function (as in non-multimethod languages). Now it is less clear how LambdaInfo is a "lambda" but SourceInfo is not.

SourceInfo also doesn't contain source code, but IR. Maybe it should be called CodeInfo? Or something containing IR?

It's also a bit hard to understand what a LambdaInfo is now. I would say its primary job is to associate a tuple type with a callable function pointer. Maybe something like CompiledFunction? In any case this would be a good opportunity to eliminate the use of "lambda" here.

vtjnash · 2016-09-09T02:39:39Z

CodeInfo was my other top name, so I can change to that. The only reason I went with SourceInfo was that source and linfo are the same number of characters, so the replacement was easier :P. This could even be the LambdaInfo type, although I agree "lambda" is a bit jargon-y and doesn't necessarily provide any additional clarity over using common words like CodeInfo.

Perhaps MethodSpecialization? It's a bit long, but I don't think this name will appear too frequently in user code. It also could be considered a leaf type realization of an Arrow, but I haven't been able to form that into a useable name.

JeffBezanson · 2016-09-09T03:06:43Z

Specialization or MethodSpecialization is a pretty good description, but the code isn't necessarily specialized. I almost want to call it Method and rename the existing Method to MethodDef. Callable might make sense, but that sounds like an abstract type. CallTarget? FunctionPointer?

vtjnash · 2016-09-09T03:53:05Z

Semantically, to some extent, "unspecialized" is a specialization (just not a leaf one). But point taken. I'm reluctant use use "Function" in the name, since I think that nomenclature is already sufficiently taken (and brings back memories of the old generic function / anonymous function confusion for new users). I agree calling this a Method isn't bad, although name shifts are a bit annoying to handle for deprecations. Another similar option might be Thunk, although again, that's perhaps too jargon-y. Another word to toss around might be "Signature".

JeffBezanson · 2016-09-09T13:05:56Z

How about CompiledMethod? It's not necessarily compiled, but conveys that it's a method that's had some processing done to it to put it into a callable form.

vtjnash · 2016-09-09T14:58:54Z

I'm not sure that's really its key attribute. I think really the only key property is that it contains a specTypes key, since it basically exists just to merge a bunch of independent caches that could instead have been in independent TypeMaps (or computed directly from sig). How about MethodSignature?

JeffBezanson · 2016-09-09T15:44:18Z

I do think it's significant that it contains actual function pointers. It's very nearly a traditional symbol table entry, mapping a name (or type in our case) to an address.

JeffBezanson · 2016-09-09T16:41:19Z

MethodInstance? Idea being that it's an instantiation of a method.

vtjnash · 2016-09-09T18:43:55Z

That sounds good. I think that also helps indicate that this is a singleton, which is also good.

…lizer fix #18449 (the real, non-buggy fix is in #18413 for v0.6-dev master)

vtjnash · 2016-09-12T17:06:23Z

Will merge once the CI build finishes (so I can resume work on #265 PR).

tkelman · 2016-09-12T17:37:08Z

does this need more deprecations? was it properly reviewed considering how much code it changes?

tkelman · 2016-09-12T18:07:09Z

base/reflection.jl

+        s = ccall(:jl_copy_code_info, Ref{CodeInfo}, (Any,), s)
+        s.code = ccall(:jl_uncompress_ast, Array{Any,1}, (Any, Any), m, s.code)
+    end
+    return s


is the maybe-aliasing here potentially an issue?

the callee doesn't own this return value. we would need to make a deepcopy if that's your concern, but this is not new.

tkelman · 2016-09-13T07:55:44Z

and if we're serious about performance tracking, any significant changes like this should run through @nanosoldier runbenchmarks(ALL, vs = ":master") first

tkelman · 2016-09-13T08:32:24Z

src/jitlayers.cpp

-            jl_finalize_function(F, collector ? collector : m.get());
+static void jl_merge_recursive(Module *m, Module *collector)
+{
+    // probably not many unresolved declarations, but be sure iterate over their Names,


be sure to iterate

nanosoldier · 2016-09-13T10:30:16Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

vtjnash · 2016-09-13T19:45:15Z

cool, @nanosoldier runbenchmarks(ALL, vs = ":master") found an (existing) bug in a previously unused value (probably a rebase error in 1dae0e0?) that ended up making the JSON test a bit slower.

due to other recent changes, LambdaInfo is now much more expensive to create and difficult to destroy it is also hard to keep track of where modification is allowed the SourceInfo type represents an atomic chunk of information that needs to be immutable after construction this sets up some of the work needed for #265, and decreases the size of the sysimg :)

this lets us avoid needing atomic operations to order the reads of fptr and jlcall_api since both will be null-initialized, set-once while the codegen lock is held

allows removing inCompile, which wasn't necessarily entirely accurate anyways for complex recursive calls from type-inference, it theoretically could have compiled part of a cycle, but not the whole cycle, and failed when it tried to finalize it, without realizing part of the cycle was still being compiled

…od idea this is required by the threadcall function

nanosoldier · 2016-09-13T22:19:34Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

tkelman · 2016-09-14T14:20:31Z

doc/devdocs/ast.rst

+
+``ftpr`` - The generic jlcall entry point
+
+``jlcall_api`` - The ABI to use when calling ``fptr``. Some significant ones include:


api vs abi still confusing terminology

Probably introduced after #18413. Re-uses the unused 'thunk' root.

StefanKarpinski added the codegen Generation of LLVM IR and native code label Sep 8, 2016

StefanKarpinski added this to the 0.6.0 milestone Sep 8, 2016

StefanKarpinski assigned vtjnash and JeffBezanson Sep 8, 2016

vtjnash force-pushed the jn/sourceinfo branch from c7bfdd7 to 094c7f6 Compare September 8, 2016 19:11

JeffBezanson reviewed Sep 9, 2016
View reviewed changes

vtjnash force-pushed the jn/sourceinfo branch 4 times, most recently from 9d0034a to 7fec8c4 Compare September 10, 2016 03:23

vtjnash added a commit that referenced this pull request Sep 12, 2016

hack in a fix to getting the right lambda for the incremental deseria…

751490d

…lizer fix #18449 (the real, non-buggy fix is in #18413 for v0.6-dev master)

vtjnash added a commit that referenced this pull request Sep 12, 2016

hack in a fix to getting the right lambda for the incremental deseria…

b90f1a2

…lizer fix #18449 (the real, non-buggy fix is in #18413 for v0.6-dev master)

vtjnash mentioned this pull request Sep 12, 2016

[release 0.5] hack in a fix to getting the right lambda for the incremental deserializer #18452

Merged

vtjnash force-pushed the jn/sourceinfo branch from 7fec8c4 to 1b22b61 Compare September 12, 2016 15:56

tkelman reviewed Sep 12, 2016
View reviewed changes

tkelman reviewed Sep 13, 2016
View reviewed changes

vtjnash force-pushed the jn/sourceinfo branch from 1b22b61 to ae32538 Compare September 13, 2016 18:29

vtjnash added 9 commits September 13, 2016 16:13

pop SourceInfo meta nodes during jl_method_set_source

6197fc8

increment jlcall_api to make 0 invalid

b66a8ff

this lets us avoid needing atomic operations to order the reads of fptr and jlcall_api since both will be null-initialized, set-once while the codegen lock is held

be more aggressive at type-inferring cfunction than is generally a go…

2bc2a10

…od idea this is required by the threadcall function

update ast devdocs to describe SourceInfo changes

25e9d91

big rename: SourceInfo -> CodeInfo, LambdaInfo -> MethodInstance

cb975ff

make jl_interpret_call partially fixed and more robust

9c51e41

fix nargs in InferenceState for varargs functions

88122b3

vtjnash force-pushed the jn/sourceinfo branch from 0c70f14 to 88122b3 Compare September 13, 2016 20:19

vtjnash merged commit 0baa867 into master Sep 14, 2016

vtjnash deleted the jn/sourceinfo branch September 14, 2016 14:07

yuyichao mentioned this pull request Sep 14, 2016

Type inference error due to precompilation #18449

Closed

tkelman reviewed Sep 14, 2016
View reviewed changes

Keno mentioned this pull request Sep 14, 2016

Gallium fails to precompile JuliaDebug/Gallium.jl#153

Open

This was referenced Sep 19, 2016

Generalize broadcast to handle tuples and scalars #16986

Merged

broadcast test failure #18577

Closed

timholy mentioned this pull request Sep 19, 2016

Break type stability of Rational with unrelated function (0.5-specific) #18465

Closed

maleadt added a commit that referenced this pull request Oct 13, 2016

Fix missing GC root.

6674671

Probably introduced after #18413. Re-uses the unused 'thunk' root.

maleadt added a commit that referenced this pull request Oct 13, 2016

Fix missing GC root.

67aa5bf

Probably introduced after #18413. Re-uses the unused 'thunk' root.

maleadt mentioned this pull request Oct 13, 2016

Fix missing GC root. #18904

Merged

jgoldfar mentioned this pull request Oct 21, 2016

Failure to Precompile Keno/ASTInterpreter.jl#61

Open

ihnorton mentioned this pull request Jun 16, 2017

Printing of Expr(:thunk) #18182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split SourceInfo out of LambdaInfo #18413

split SourceInfo out of LambdaInfo #18413

vtjnash commented Sep 8, 2016

JeffBezanson Sep 9, 2016

vtjnash Sep 9, 2016

JeffBezanson Sep 9, 2016

vtjnash Sep 9, 2016

JeffBezanson Sep 9, 2016

JeffBezanson commented Sep 9, 2016

vtjnash commented Sep 9, 2016

JeffBezanson commented Sep 9, 2016

vtjnash commented Sep 9, 2016

JeffBezanson commented Sep 9, 2016

vtjnash commented Sep 9, 2016

JeffBezanson commented Sep 9, 2016

JeffBezanson commented Sep 9, 2016

vtjnash commented Sep 9, 2016

vtjnash commented Sep 12, 2016

tkelman commented Sep 12, 2016

tkelman Sep 12, 2016

vtjnash Sep 13, 2016

tkelman commented Sep 13, 2016

tkelman Sep 13, 2016

nanosoldier commented Sep 13, 2016

vtjnash commented Sep 13, 2016

nanosoldier commented Sep 13, 2016

tkelman Sep 14, 2016

	if inferred && code.inferred && linfo !== code
	# This case occurs when the IR for a function has been deleted.
	# `code` will be a newly-created LambdaInfo, and we need to copy its
	# contents to the existing one to copy the info to the method cache.
	linfo.inInference = true
	linfo.code = code.code
	linfo.slotnames = code.slotnames
	linfo.slottypes = code.slottypes
	linfo.slotflags = code.slotflags
	linfo.ssavaluetypes = code.ssavaluetypes
	linfo.pure = code.pure
	linfo.inlineable = code.inlineable
	linfo.propagate_inbounds = code.propagate_inbounds
	ccall(:jl_set_lambda_rettype, Void, (Any, Any), linfo, code.rettype)
	if code.jlcall_api == 2
	linfo.constval = code.constval
	linfo.jlcall_api = 2
	end
	linfo.inferred = true
	linfo.inInference = false
	end


		``ftpr`` - The generic jlcall entry point

		``jlcall_api`` - The ABI to use when calling ``fptr``. Some significant ones include:

split SourceInfo out of LambdaInfo #18413

split SourceInfo out of LambdaInfo #18413

Conversation

vtjnash commented Sep 8, 2016

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

JeffBezanson commented Sep 9, 2016

vtjnash commented Sep 9, 2016

JeffBezanson commented Sep 9, 2016

vtjnash commented Sep 9, 2016

JeffBezanson commented Sep 9, 2016

vtjnash commented Sep 9, 2016

JeffBezanson commented Sep 9, 2016

JeffBezanson commented Sep 9, 2016

vtjnash commented Sep 9, 2016

vtjnash commented Sep 12, 2016

tkelman commented Sep 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Sep 13, 2016

Choose a reason for hiding this comment

nanosoldier commented Sep 13, 2016

vtjnash commented Sep 13, 2016

nanosoldier commented Sep 13, 2016

Choose a reason for hiding this comment