Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Byte alignment, optimization from 96 to 80 byte #865

Merged
merged 2 commits into from
Dec 11, 2020

Conversation

wziww
Copy link
Contributor

@wziww wziww commented Sep 18, 2020

Optimization Logger struct

@CLAassistant
Copy link

CLAassistant commented Sep 18, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution @wziww

The change is correct, but it has some unnecessary reordering. For example, there's no advantage in moving core to not be the first field, and having it there is a little cleaner.

The main improvement is from grouping the bool types + the CheckWriteAction, no need to reorder the zapcore.Core.

@wziww
Copy link
Contributor Author

wziww commented Oct 9, 2020

Thank you for the contribution @wziww

The change is correct, but it has some unnecessary reordering. For example, there's no advantage in moving core to not be the first field, and having it there is a little cleaner.

The main improvement is from grouping the bool types + the CheckWriteAction, no need to reorder the zapcore.Core.

Yeah you right.

@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #865 into master will decrease coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #865      +/-   ##
==========================================
- Coverage   98.36%   98.21%   -0.16%     
==========================================
  Files          43       43              
  Lines        2390     1904     -486     
==========================================
- Hits         2351     1870     -481     
+ Misses         32       27       -5     
  Partials        7        7              
Impacted Files Coverage Δ
logger.go 96.15% <ø> (+0.84%) ⬆️
internal/ztest/timeout.go 81.81% <0.00%> (-4.85%) ⬇️
zapcore/write_syncer.go 87.50% <0.00%> (-2.98%) ⬇️
internal/exit/exit.go 90.90% <0.00%> (-2.85%) ⬇️
zapcore/encoder.go 87.09% <0.00%> (-1.14%) ⬇️
zapcore/core.go 93.54% <0.00%> (-1.05%) ⬇️
zapcore/entry.go 93.82% <0.00%> (-0.80%) ⬇️
stacktrace.go 96.00% <0.00%> (-0.56%) ⬇️
global.go 96.66% <0.00%> (-0.48%) ⬇️
flag.go 100.00% <0.00%> (ø)
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 404189c...a3911a3. Read the comment docs.

@rabbbit
Copy link
Contributor

rabbbit commented Oct 9, 2020

Is there an easy/automated way you were aligning this, or is it just manual checking? :)

@wziww
Copy link
Contributor Author

wziww commented Oct 10, 2020

Is there an easy/automated way you were aligning this, or is it just manual checking? :)

I do think manual checking is the best, try to form a habit while coding.
But we can also use reflect to build some small tools to check it.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants