Skip to content

RFC re alternative approach to #370 #384

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dlmiles
Copy link
Contributor

@dlmiles dlmiles commented Feb 19, 2025

This PR is a Request For Comments on these alternative commits related to #370

In PR #370 I invalidate with *pathheadpp = NULL but this is of course extra work the CPU has to do.

So here is a V1 and V2 alternative, that achieves multiple things:

  • make both functions easier for the next person to understand/reason about (understand this maybe somewhat opinionated)
  • reduces the amount of work the CPU needs to do overall (true of V2 not of V1), mainly from the management on both sides of the function call for having 2 piece of data, return value and CIFPath*
  • -O3 compiler headline in respective commit comment (of the function implementation in situation inside magic)
  • quashes (false positive) static code analysis report concerning this (this is an issue, in that its easier to do something in the code than manage the extensive lists of points picked up)

just making a case to understand what kind of factors justify

At the moment consider the patch untested, looking to see if there is understanding that in win-win-win (readability-performance-concern_management) scenarios.
If there is a positive feedback I shall get to testing and report back in issue (could be days/weeks)
Thanks

Under -O3: +1 branch, +0x18 code size (than original under -O3)

Easier to reason about, there can be no interaction from *pathheadpp
and the various functions called, which maybe the first concern to
the next reader as visibility of new data is limited to that of a
local variable of the function.
Under -O3: -2 branches, same code size (than original under -O3)

I assume the (from exp) callers also get easier time, as there is just
one piece of data to manage from the return (not 2).  Compiler gets on
where to put the return value.
@dlmiles
Copy link
Contributor Author

dlmiles commented Feb 20, 2025

PR currently exists for feedback would welcome review, to understand alternative viewpoint around change of this nature.

Merge Status: merge on hold (discussion only)
Quality: good enough to dicuss
Risk: low-impact (should not change any functionality)
Level of Testing: will receive targeted testing pending a favourable review to proceed

A favourable review here would have me amend #370 to remove *pathheadpp = NULL changes and push the reorder on its own. Then rework this PR to be production ready after testing.
An unfavorable review here would have me close this and remove the *pathheadpp = NULL changes and push the reorder on its own.

Copy link
Owner

@RTimothyEdwards RTimothyEdwards left a comment

Choose a reason for hiding this comment

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

After reviewing the code, I think this approach is fine. I don't see any way in which the result is not redundant to the check of pathheadp being NULL or non-NULL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants