-
Notifications
You must be signed in to change notification settings - Fork 367
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
coverage: count number of executions per line #1265
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, I would just benchmark if this doesn't add too much overhead
I think either the workers will be frequently sending events (quite bad) or some counter will not be properly updated (not so bad, still not great). |
This is awesome, I want this feature so bad 😆 |
6c0a717
to
97007bc
Compare
I did a second attempt at implementing this a safer way. The code is quite ugly right now but if anyone wants to test/review it before I spend some more time on it I'd appreciate it 👍 |
I don't see this branch under Actions to download the binary 🤔 |
I think it is because it is a draft, let me convert it a proper PR |
97007bc
to
559bbc1
Compare
@rappie There was a small git conflict, hopefully it builds now that I resolved it 👍 |
0b5d6a0
to
8d2cb5e
Compare
@samalws-tob can you take a look to this PR? |
@@ -304,13 +314,13 @@ execTxWithCov tx = do | |||
-- bug in another place, investigate. | |||
-- ... this should be fixed now, since we use `codeContract` instead | |||
-- of `contract` for everything; it may be safe to remove this check. | |||
when (pc < VMut.length vec) $ | |||
when (pc < VMut.length vec) $ do | |||
VMut.modify (fromJust maybeStatsVec) (\(execQty, revertQty) -> (execQty + 1, revertQty)) opIx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see anywhere where we increment revertQty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I have not implemented counting of reverts. I meant to do it at first but then it wasn't clear to me where I should be counting them. If you have any pointers there I'd appreciate them, otherwise I may remove the extra var at this time.
839d2d3
to
924dfb2
Compare
Revert counting is still not implemented.
I benchmarked this, looks like it cuts performance (calls/s) in half |
in #1305 tried doing |
I'm still very interested in this, despite the performance loss. It would be great to have this in |
No description provided.