-
Notifications
You must be signed in to change notification settings - Fork 248
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
[query] Use sourcecode.Enclosing
to handle timed blocks implicitly
#14683
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
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.
Here's what I noticed in my first pass. I think it's probably, pretty good otherwise.
hail/src/main/scala/is/hail/expr/ir/lowering/LoweringPass.scala
Outdated
Show resolved
Hide resolved
a11a232
to
098f6a4
Compare
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'm satisfied. Helps clean things up a bunch. Thanks!
1 similar comment
098f6a4
to
2b028ba
Compare
2b028ba
to
8c7b1c3
Compare
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 is a really great change! I have a few small comments/requests, but overall I'm excited to get this merged!
Example timing output:
|
8c7b1c3
to
3c702c0
Compare
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.
Responded in threads
8fb6e01
to
2f05ecd
Compare
2f05ecd
to
83ff2cc
Compare
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.
Thanks again for the great change, and sorry for the slow review!
Change Description
This change exists as part of larger refactoring work. Herein, I've exchanged
hard-coded contextual strings passed to
ExecutionTimer.time
with implictcontexts, drawing inspiration from scalatest.
These contexts are now supplied after entering functions like
Compile
andEmit
instead of before (seeExecuteContext.time
). By sprinking calls totime
throughout the codebase after entering functions, we obtain a nice traceof the timings with
sourcecode.Enclosing
, minus the previous verbosity.See [1] for more information about what pre-built macros are available. We can
always build our own later. See comments in [pull request id] for example output.
Note that
ExectionTimer.time
still accepts a string to support uses likeOptimise
andLoweringPass
where those contexts are provided already.It is also exception-safe now.
This change exposed many similarities between the implementations of query
execution across all three backends. I've stopped short of full unification
which is a greater work, I've instead simplified and moved duplicated result
encoding into the various backend api implementations.
More interesting changes are to
ExecuteContext
, which now supportstime
, as discussed abovelocal
, a generalisation for temporarily overriding properties of anExecuteContext
(inspired by [2]). While I've long wanted this for testing,we were doing some questionable things when reporting timings back to python,
for which locally overriding the
timer
of actx
has been very useful.We also follow this pattern for local regions
[1] https://github.com/com-lihaoyi/sourcecode
[2] https://hackage.haskell.org/package/mtl-2.3.1/docs/Control-Monad-Reader.html#v:local
Security Assessment
This change has no security impact as it's confined to refactoring of existing non-security-related code.