Fix performance degradation: make message fields lazy #8603
Conversation
|
We should also update the advice how to write an error message to include the laziness. And be vigilant for a while that nothing strict slips back in. The rules are:
|
|
test performance with #fast please |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
| @@ -12,7 +12,7 @@ object Message { | |||
| * not yet been ported to the new scheme. Comment out this `implicit def` to | |||
| * see where old errors still exist | |||
| */ | |||
| implicit def toNoExplanation(str: String): Message = | |||
| implicit def toNoExplanation(str: => String): Message = | |||
There was a problem hiding this comment.
It's worth noting that we cannot have an implicit conversion from a by-name param when using Dotty's Conversion mechanism.
There was a problem hiding this comment.
Yes, I noted that as well. That's a problem.
|
performance test failed: Please check http://lamppc37.epfl.ch:8000/pull-8603-03-24-21.43.out for more information |
I connected to EPFL vpn to check it out, but got back:
/cc @liufengyun |
|
It's running now --- I accidentally remove the |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/8603/ to see the changes. Benchmarks is based on merging with master (6cd3a9d) |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/8603/ to see the changes. Benchmarks is based on merging with master (6cd3a9d) |
|
Why not make all the message fields ( |
|
@allanrenucci Good point. I don't know whether there are situations where we want to read these fields more than once. |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
performance test failed: Please check http://lamppc37.epfl.ch:8000/pull-8603-03-25-19.27.out for more information Merge conflict, the PR needs rebase. |
Make field `msg` and `explanation` in Message lazy and make message containers strict. That way, we can look inside a message container to find out what kind of message we are having without fear of forcing too much.
Also: - Turn some message classes into normal classes. More to follow. For error messages there's really no justification to spend all the extra code size on case class infrastructure. - Drop ErrorMessageTest. That's the only test that requires message classes to be case classes. Over the whole time of its existence, that test was nothing more than a pain in the ass. I do not think there was a single real fault that it reported but it caused lots of extra work everytime a message changes. Best not to waste more cycles composing and fixing these tests.
There's no need to have the additional code for the case class machinery in the compiler.
Fixes in files that were not saved before
It was not clear go me that `diagnostic` should be its own package.
The previous pipeline forgot to filter out <nonsensical> tags in most situations
odersky
force-pushed
the
fix-performance
branch
from
5780ff5 to
dd8af22
Compare
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/8603/ to see the changes. Benchmarks is based on merging with master (37eec14) |
Caching is done at a central point in class `Message` where also `<nonsensical>` tags are dropped.
odersky
force-pushed
the
fix-performance
branch
from
e36cfed to
a535680
Compare
|
@anatoliykmetyuk can you give it a quick review? Since it touches many things it would be good to get this in sooner rather than later. It might still have some weaknesses but I am sure it's better than what we had before. |
Make fields
msgandexplanationin Message lazy and makemessage containers strict.
That way, we can look inside a message container to find out what kind
of message we have without fear of forcing too much.
[EDIT]
This has turned into a large-scale refactoring of the error reporting chain. The aim was to make things more consistent, simpler, and more robust. We also fix some problems where tags would make it into the reported message, which was a classic case of people working on paralellel solutions that did not compose correctly with each other.
One reason for the drop in linecount is that I chucked ErrorMessagesTest out. I had little else but nuisance value.