Optimizations & Chinese i18n #246
Conversation
|
I am not sure why the check failed. Seems to point to a memory issue? |
LaCuneta
left a comment
There was a problem hiding this comment.
Thanks for putting this together.
Did you do any benchmarks to see what the effect of replacing the function calls with their unrolled code would be for gt(), lt() and the patchRightAndAhead() calls? I'm willing to include them either way, but very curious what kind of speed-up they would generate.
I left some comments inline with some small issues. Please sign and date your code comments when you add them.
What is the purpose for adding messageKey to the RuntimeException? I don't see that used anywhere. If it's not really doing anything, I'd prefer to remove it.
Let me know if you have any questions.
|
Generally, I think the performance boost of gte and lte could be significant but probably not patchAtHeading stuff. If you look at how the gte was implemented before, the performance would actually depend on how likely the check is going to fail - For example, in
Considering the many branches in eq, and many irrelevant branches, the boost could be considerable. Alas, we do need to have a comprehensive benchmark that runs on a set of models... If time permits, I will find a VRF intern to work on that. |
|
Just added comments as requested. Thanks! |
|
To answer your question What is the purpose for adding messageKey to the RuntimeException? I don't see that used anywhere. If it's not really doing anything, I'd prefer to remove it. The purpose is to let TU know the exception message BEFORE localization - so that we can add other help information for learners to debug the issue. Without the message key, it is harder to decipher since errors are multilingual and with template parameters now. |
Gotcha, that makes sense. Now that I understand that I looked back at it and I think something is missing to complete that change. The |
|
Hey John, the last couple commits had messages that could be more helpful. We'd prefer to keep our commit messages meaningful on their own, so "Jeremy's request" wouldn't mean much without the context of this PR. The "Merge branch..." one is similarly not descriptive of the actual changes there. You don't need to amend those commits and force push, but just letting you know for future reference. |
|
|
||
| # (String, String, Maybe[Int], Maybe[Int]) => RuntimeException | ||
| runtime: (message, primitive, sourceStart = None, sourceEnd = None) -> | ||
| # Most of the existing call to this function have not been changed to i18n + messageKey. |
There was a problem hiding this comment.
The linter error'd for trailing whitespace here - the CoffeeScript linter isn't smart enough to see it's a comment. I turn on the setting in my editor to always strip these and to add the end-of-file newline, which helps avoid the issues.
The optimizations might cause a bit more redundant code, but they are very frequently executed, so I believe they are worthwhile.
Note that I removed a duplicate key-value pair when translating the English error messages to Chinese.
There is also a TU-specific error message but in the future, the features might be brought back to NLW as an extension.
We probably need to think about a way for extension i18n when we open up NLW extensions to the public.