-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix: faster compilation for JSON::MappingError #6111
Conversation
Would be nice to have benchmarks for compiling your mapping test using the old stdlib, the |
It doesn't matter if it's master or not. Try compiling the code I posted in the other issue with this PR and without it, and you'll see the difference. |
It would be nice to find other places in the standard library and compiler where simple changes like this improve compilation speed, but I don't know how to detect that. Maybe a tool that would show how many instantiations a method has, sorted by that. |
@asterite, I've been messing with this piece of code that shows all instantiations, and sorting the results by count seems easily doable. |
@oprypin Yup, I was going to mention you because I know you've been playing with it. I think Here's a diff to quickly try this out Here's the result when trying out the code before this PR:
Hehe, it seems that was the culprit after all. 300 instantiations of With this PR:
But for some reason I also see:
I'll investigate that a bit, though now I'm curious to know which methods are troublesome in the compiler... |
I updated the gist and it now shows percentages. So now we can see:
That represents half of the total instantiations in the program. Now wonder it was 1 second slower...
With the compiler I see:
Looots of redundant code generated, I guess. I'm also seeing many primitives being instantiated, that's something that might not be correct, I'll investigate that too. I don't know what to think about this. Would it be good to have such tool, so when compile times become big you'll refactor your code so that it compiles faster? I don't know of another language where you'd have to do that, and I don't know how annoying it could be... or how to preserve those "nice" compile times later and not have someone accidentally screw all of that. |
How much harder would it be to measure the time spent on each instantiation as well? |
@RX14 That's much harder. An instantiation will instantiate other stuff... I don't know how to measure that. |
It would be sooo much easier if all methods were typed. Then there would be just one instantiation per method definition, unless the method is "generic" ( |
@asterite is there one "entry point" method for the start of an instantiation? In that case since we're single-threaded we can store the current instantiation in a stack in a global variable, and associate the instantiation at the top of the stack with a time. So at the start of the instantiation entrypoint, we:
And yes, it's much easier if all methods were typed. That's how every other language does it and it's why crystal is an order of magnitude more expressive than those languages. |
@RX14 Yeah, that probably can be done. Not sure what we'll use it for, though. One thing I see that could be improved, probably without a language change, is how methods are instantiated regarding With this code: class Foo
def foo
end
end
class Bar < Foo
end
Foo.new.foo
Bar.new.foo The (updated, it had a bug because instantiations didn't take into account the real type that hold the method, that's why there were 38 instantiations of Pointer(UInt8)#value) tool tells:
Now that there's For example: class Foo
def foo
virtual
end
def virtual
1
end
end
class Bar < Foo
def virtual
"foo"
end
end
p typeof(Foo.new.foo) # => Int32
p typeof(Bar.new.foo) # => String That's closer to Ruby, because if you do The disadvantage is that if there's a huge class hierarchy (like that of Maybe in this case the compiler should generate the method once, passing self as |
Ouch, sounds like a big disadvantage to me. |
Passing
Class
as an argument is a really bad idea: the method will be instantiated once for each class that invokesJSON.mapping
. This makes compilation really slow. Also, so many overloads are not necessary, the class is only used in that file in four places.Related to #6082 (comment)