Skip to content
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

Timing: add generated code jit timing feature #180

Merged
merged 2 commits into from
Jul 3, 2023

Conversation

JakeHillion
Copy link
Contributor

@JakeHillion JakeHillion commented Jun 23, 2023

Summary

Adds a timing call at the beginning and end of the generated code functions and store the time taken in nanoseconds in the data segment. Currently this is output at LOG(INFO) level. IMO this belongs in RocksDB and oid_out.json, but it requires a bit of a format change so I'll leave it until it's necessary.

This required adding another uintptr_t typed element to the data segment prefix, because adding it at the end of the data segment means keeping overhead of setting lengths and such after the call. Probe effect should be minimal as is, but as it's feature gated and off by default I haven't bothered measuring it. I added some names to pieces in these entry functions while I was in there, and also cleaned up a type in the typed data segment entry.

Test plan

  • CI
  • []: make test: 100% tests passed, 0 tests failed out of 578
  • ["-fjit-timing"]: make test: 100% tests passed, 0 tests failed out of 578
  • ["-ftype-graph"]: make test: 95% tests passed, 31 tests failed out of 578
  • ["-ftype-graph", "-fjit-timing"]: make test: 95% tests passed, 31 tests failed out of 578
  • ["-ftyped-data-segment"]: make test: 87% tests passed, 77 tests failed out of 578
  • ["-ftyped-data-segment", "-fjit-timing"]: make test: 87% tests passed, 77 tests failed out of 578
    (that was hard work)

Copy link
Contributor

@ajor ajor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cppreference doesn't have a high opinion of high_resolution_clock:

The high_resolution_clock is not implemented consistently across different standard library implementations, and its use should be avoided.
...
Generally one should just use std::chrono::steady_clock or std::chrono::system_clock directly instead of std::chrono::high_resolution_clock: use steady_clock for duration measurements, and system_clock for wall-clock time.

Copy link
Contributor

@ttreyer ttreyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks mostly good to me 🙂
Could you change the bool flag to the actual feature set, as discussed, please.
Also take a look at the data[3] idea.

oi/FuncGen.cpp Show resolved Hide resolved
@JakeHillion JakeHillion force-pushed the timing branch 3 times, most recently from e26d3bc to 2d2ac0c Compare June 26, 2023 14:22
@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2023

Codecov Report

Merging #180 (47fa73c) into main (f130f3c) will decrease coverage by 0.12%.
The diff coverage is 36.36%.

❗ Current head 47fa73c differs from pull request most recent head 4e2d019. Consider uploading reports for the commit 4e2d019 to get more accurate results

@@            Coverage Diff             @@
##             main     #180      +/-   ##
==========================================
- Coverage   62.63%   62.51%   -0.12%     
==========================================
  Files          87       87              
  Lines        9266     9290      +24     
  Branches     1533     1542       +9     
==========================================
+ Hits         5804     5808       +4     
- Misses       2639     2652      +13     
- Partials      823      830       +7     
Impacted Files Coverage Δ
oi/OIDebugger.h 45.28% <ø> (ø)
oi/FuncGen.cpp 59.85% <30.00%> (-5.19%) ⬇️
oi/OIDebugger.cpp 38.07% <33.33%> (-0.02%) ⬇️
oi/CodeGen.cpp 38.24% <40.00%> (-0.22%) ⬇️
oi/OICodeGen.cpp 67.02% <50.00%> (-0.10%) ⬇️
oi/TreeBuilder.cpp 76.28% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@JakeHillion JakeHillion requested a review from ttreyer June 26, 2023 14:33
@JakeHillion JakeHillion force-pushed the timing branch 3 times, most recently from 4e2d019 to e2c4e49 Compare June 27, 2023 16:01
@ajor ajor removed the request for review from ttreyer July 3, 2023 16:42
@JakeHillion JakeHillion merged commit 01c9573 into facebookexperimental:main Jul 3, 2023
@JakeHillion JakeHillion deleted the timing branch July 3, 2023 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants