-
Notifications
You must be signed in to change notification settings - Fork 107
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
CatchableError static exception tracking pollited by base class Exception #636
Comments
Can you provide a minimal example for this? I haven't stumbled on such a problem. In general, you shouldn't be shy about introducing changes in the vendor packages that improve the exception tracking. The infectious general |
Example:
Playing with that assignment and with the initConfiguration() implementation led me come to that conclusion. |
I'm not convinced that this is a useful objective, as the purpose appears to be to wrap all other exception that can be produced into This is more obvious with EVMC, where the status is limited to a set of codes from an enum. It would make more sense to turn exceptions into the correct status, instead of returning a general error code and losing the information. Although I do see the conceptual cleanliness motivation, there is already a Considering the enthusiasm you showed for jump-table micro-optimisation, whose benefit is lower than the cost of If I have this all wrong, and the point is only to wrap exceptions around calls to host services like the database etc, that's different and the motivation for wrapping is clearer. In that case, though, you know the non-EVMC calls are going away, so all calls will be through the |
You're measuring the test suite, not the EVM execution time. Much of that is JSON parsing, blockchain construction, hashing, database storage, reading 1.2 GB of small files from the OS, etc. all of which are not part of EVM. Probably 0.6% you saw is noise (it's marginal), unless you averaged over many runs and randomly alternated branches to avoid stuck performance externalities. But if it's truly 0.6% the EVM performance will be different by a larger percentage than that. |
Well, it's also reasonable to define a The primary goal of my suggestion is to ensure that all errors are being handled. The specific |
There is an All errors are already handled in some sense, because the interpreter loop has That's what I mean when I say any I agree performance comes after correctness, but I'm missing the correcness issue that needs solving here. If instead we were talking about |
In general, the bar for "special reasons" is pretty high - ie it should have a demonstrated and concrete benefit which is not "I don't like seeing error handling code" - we have a policy of explicit error handling in place because the error cases are considered equally important to non-error cases. In very very rare cases, performance might be one such reason - but even in these cases, each "logical" module should ensure that the errors that leak out of it are contained and don't infect outside code, and don't get infected by transitive dependencies in an uncontrolled manner. |
I'm not clear on who is taking what position on this, as the conversation is confusing, so I'll sum up some facts which are pretty much how it has to be.
Also this, listed separately because it's an opinion not fact:
|
The original issue here was about "raw" (base class) Exceptions leaking. I don't like the proposed solution for this with two different modes. If they can't be dealt with immediately, one approach is to Regarding the "random" exceptions, I agree with @arnetheduck his points there. But this is really more of a lesser problem if they are dealt with already compared to the raw Exceptions where you are not sure about what might cause them. |
The goal of the raises annotations is to make the exceptions more apparent in the code. If you have a general In other words, when you start adding raises annotations you are often making trivial discoveries such as "oh, parseInt can raise a ValueError, I should handle it here". |
Yeah, completely agree on this. Saying "we are not letting any exceptions pass at the API call level so its fine" is not necessarily enough, as you might just not be aware of the possibility of a certain type of errors and thus the way they should be taken care off. I'm not sure if that is the case here as as don't know the code. And it is understandable for now considering legacy etc. I'm just trying to point out that getting rid of raw Exceptions is probably the better first step. |
Closed as problem has become outdated with nim v1.2.16 |
Are there some idea, comments on better solution for the following problem?
Objective
The OP handlers for the re-factored VM2 should be annotated with raises: [Defect,EVMError]
Problem
Some vendor modules leak potential base class Exception exceptions, most annoyingly from the eth/trie modules. Also, ref object assignment seems to flag an Exception at compile time analysis (the nimbus style guide suggests an init() function.) See https://forum.nim-lang.org/t/6783#42459 for some discussion on that topic (I also realised that the not nil object attribute seems to have gone which never worked too well anyway.)
Partial Solution Applicable to Current Code
As a partial solution without extensive code changes, I implemented the following in the branch feature/vm2-annotate-raises-exceptions:
So this allows a static exception analysis for the debug version with comparable exception run-time behavior. BTW, running the test suite does not result in much execution time penalties (observed ~0.6%.)
The text was updated successfully, but these errors were encountered: