Skip to content

Conversation

jketema
Copy link
Contributor

@jketema jketema commented Sep 10, 2024

Note that although this fixes some inconsistencies this does not (yet) accurately reflect the semantics of __finally blocks. See the highlighted paragraph here.

Both DCA experiments are relevant. DCA shows a 10%~11% slowdown on keepassxc. It' not clear to me where this is coming from. I locally tried rerunning and I do see a small slowdown locally too (< 10%, but I was running single threaded), and the largest joins are identical between the two versions. So, I'm not sure what to do with this.

@github-actions github-actions bot added the C++ label Sep 10, 2024
@jketema jketema force-pushed the fix-finally-inconsistency branch from da61e85 to 5754f8b Compare September 10, 2024 18:39
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

The code changes LGTM! I'm surprised there are no reported IR inconsistency fixes in DCA, but I suppose we simply don't have any projects that use __try/__finally 😭

(I think this is a great PR to review in case someone wants to learn more about IR construction.)

@jketema jketema marked this pull request as ready for review September 11, 2024 13:15
@jketema jketema requested a review from a team as a code owner September 11, 2024 13:15
@jketema jketema requested a review from geoffw0 September 11, 2024 13:15
@jketema jketema added the no-change-note-required This PR does not need a change note label Sep 11, 2024
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Less inconsistencies is great. 🎉

I have nothing to add about the QL.

As for the slowdown in keepassxc, my best guess is that we're producing a chunk more data somewhere and the extra compute is being spread out among many predicates, so you're not seeing a spike. I've had no luck narrowing down what that extra data could be (my theory was less unreachable code).

@geoffw0
Copy link
Contributor

geoffw0 commented Sep 12, 2024

I'm not seeing any slowdown locally, running queries like cpp/double-free that were highlighted in the Stage Timings report. How did you reproduce it locally? How confident are you that it's a real slowdown?

@jketema
Copy link
Contributor Author

jketema commented Sep 12, 2024

I'm not seeing any slowdown locally, running queries like cpp/double-free that were highlighted in the Stage Timings report. How did you reproduce it locally? How confident are you that it's a real slowdown?

I ran the whole nightly suite. The few reruns I did seemed to consistently show:

  • Before:
    [2024-09-11 15:09:45] Total evaluation times for this run:
            * Wall-clock duration of evaluation run: 488.8 seconds
            * Total time spent evaluating predicates: 345.5 seconds
    
  • After:
    [2024-09-11 14:55:09] Total evaluation times for this run:
            * Wall-clock duration of evaluation run: 495.2 seconds
            * Total time spent evaluating predicates: 345.5 seconds
    

So, much less then 10% looking at the wall-clock time. Curiously the predicate evaluation time is identical here.

@jketema
Copy link
Contributor Author

jketema commented Sep 13, 2024

@geoffw0 Maybe I'm a bit too paranoid here? If so, I'd like to go ahead and merge this 😅

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

I've nothing against merging this.

Might be worth checking the next nightly DCA though, and bring this up if a similar regression is observed.

@jketema
Copy link
Contributor Author

jketema commented Sep 13, 2024

Might be worth checking the next nightly DCA though, and bring this up if a similar regression is observed.

Will do!

@jketema jketema merged commit 087a848 into github:main Sep 13, 2024
15 of 16 checks passed
@jketema jketema deleted the fix-finally-inconsistency branch September 13, 2024 13:27
@jketema
Copy link
Contributor Author

jketema commented Sep 13, 2024

I've nothing against merging this.

Thanks for your input!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants