-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Measure compile time only when using time macros pt.2: handling when code under test throws and make compile timing thread-local #38915
Measure compile time only when using time macros pt.2: handling when code under test throws and make compile timing thread-local #38915
Conversation
Are there any race condition concerns when calling |
1.5.3
I'm working on a print lock & compile timer activation lock for this PR |
I just meant if there is a race condition writing to the variable then (= undefined behaviour). |
Yes, sorry, I did understand that. I also noticed that this PR increases allocations due to the |
cc2180a
to
5aaa970
Compare
end | ||
parens && print(")") | ||
nothing |
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.
The diff is deceptively messy here. I just wrapped time_print
's code in a print lock, and added the newline
arg to print a newline within the print lock at the end
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.
timev_print
still prints its own newline outside the lock. If it's important for the newline to be printed inside the lock, we should do it always. If it's not, we should do it never and the caller can print a newline if wanted.
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.
For timev_print
I've switched to using the internal newline print. From what I can tell time_print
needs optionally do both it's used during the julia build process in sysimage.jl
@KristofferC the setting of I've returned to the previous non-reporting when errors throw as there doesn't seem to be a need to change that
The report printing is also now protected by a lock to make threaded printing look better.
Although, the first comp time % report here is surprisingly nonsensically high.. Compilation on multiple threads is evidently confusing the singular cumulative counter. I don't see a simple fix. Though I guess it's ok to leave as-is and tackle that in a future PR |
The compilation timing is now thread-local, without the need for locks, and seems to behave well:
|
@JeffBezanson I think this should be ready for another review |
This appears to have broken the tests (https://build.julialang.org/#/builders/5/builds/1156) |
…code under test throws and make compile timing thread-local (JuliaLang#38915) * ensure compile timing disables in time & timev macros * make comp measurement switching threadsafe
#38885 intended to only enable compile time during the timing macros, however as @ericphanson pointed out if the code under test throws the timing could be left enabled.
The fix here of using a try-finally has the side effect of enabling the timing report even when errors throw, which didn't happen previously. That could be suppressed if the previous behavior is desired.
master
This PR