🌱 Some more cleanup for field placement #3791
Conversation
- Cleanup for fieldplacement in struct Signed-off-by: naveensrinivasan <[email protected]>
naveensrinivasan
requested review from
raghavkaul
and removed request for
a team
naveensrinivasan
temporarily deployed
to
gitlab
GitHub Actions
Inactive
naveensrinivasan
temporarily deployed
to
integration-test
GitHub Actions
Inactive
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3791 +/- ##
==========================================
+ Coverage 66.61% 70.52% +3.90%
==========================================
Files 230 230
Lines 15622 15622
==========================================
+ Hits 10407 11017 +610
+ Misses 4599 3932 -667
- Partials 616 673 +57 |
| // | ||
| //nolint:govet | ||
| type CheckResult struct { | ||
| Name string | ||
| Version int |
There was a problem hiding this comment.
similar comment on human-readability. We should decide if we care about this
There was a problem hiding this comment.
Most of the time, Scorecard is used by non-humans. So it helps to have performance IMO. Thoughts?
There was a problem hiding this comment.
So it helps to have performance IMO. Thoughts?
My first thought is, does it matter? I know we have the linter (and could disable it if the answer is no), but is this a performance bottleneck? Compared to the I/O and API operations we need to wait for anyway?
There was a problem hiding this comment.
The compiler utilizes the L1 cache based on the word size.
https://articles.wesionary.team/simple-data-alignment-technique-to-speed-up-your-struct-in-golang-954c43faa1fe
https://www.ardanlabs.com/blog/2013/07/understanding-type-in-go.html
It helps to make it better.
I understand API Operations.
There was a problem hiding this comment.
The reason I ask for benchmarks is it's not quite as simple:
golang/go#10014 (comment)
That seems like it is trying to solve a 1970s problem, namely packing structs to use as little space as possible. The 2010s problem is to put related fields near each other to reduce cache misses
| type RawResults struct { | ||
| BinaryArtifactResults BinaryArtifactData | ||
| Metadata MetadataData | ||
| BranchProtectionResults BranchProtectionsData | ||
| CIIBestPracticesResults CIIBestPracticesData | ||
| CITestResults CITestData | ||
| CodeReviewResults CodeReviewData | ||
| PinningDependenciesResults PinningDependenciesData | ||
| SecurityPolicyResults SecurityPolicyData |
There was a problem hiding this comment.
So I know this was alphabetized recently to find the data easier.
#3540
There was a problem hiding this comment.
|
This pull request is stale because it has been open for 10 days with no activity |
| // | ||
| //nolint:govet | ||
| type CheckResult struct { | ||
| Name string | ||
| Version int |
There was a problem hiding this comment.
The reason I ask for benchmarks is it's not quite as simple:
golang/go#10014 (comment)
That seems like it is trying to solve a 1970s problem, namely packing structs to use as little space as possible. The 2010s problem is to put related fields near each other to reduce cache misses
|
This pull request has been marked stale because it has been open for 10 days with no activity |
|
This pull request has been marked stale because it has been open for 10 days with no activity |
|
This pull request has been marked stale because it has been open for 10 days with no activity |
What kind of change does this PR introduce?
(Is it a bug fix, feature, docs update, something else?)
What is the current behavior?
What is the new behavior (if this is a feature change)?**
Which issue(s) this PR fixes
Special notes for your reviewer
Does this PR introduce a user-facing change?
For user-facing changes, please add a concise, human-readable release note to
the
release-note(In particular, describe what changes users might need to make in their
application as a result of this pull request.)