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.

Sign up for GitHub

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

Jump to bottom

reorder struct fields for compactness #23

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

reorder struct fields for compactness #23

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
11dd7a0
reorder struct fields for compactness using https://github.com/dkorun…
ralph-tice Jun 9, 2023
312f976
revert chanegs to send and config
ralph-tice Jun 9, 2023
3cea942
add notes for flow and custom about their microoptimized field orders
ralph-tice Jun 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,21 +17,21 @@ import (

// Config describes the libkflow configuration.
type Config struct {
email string
token string
capture Capture
logger interface{}
metrics *url.URL
flow *url.URL
proxy *url.URL
api *url.URL
flow *url.URL
metrics *url.URL
sample int
timeout time.Duration
retries int
logger interface{}
program string
email string
token string
version string

metricsPrefix string
capture Capture
sample int
timeout time.Duration
retries int
metricsInterval time.Duration
}

Expand Down
110 changes: 55 additions & 55 deletions flow/flow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,73 +17,73 @@ type Ckflow C.kflow
const MAX_CUSTOM_STR_LEN = 384

type Flow struct {
TimestampNano int64
DstAs uint32
DstGeo uint32
DstMac uint32 // IGNORED - use DstEthMac
HeaderLen uint32
SrcBgpCommunity string
aaronfuj marked this conversation as resolved.
Show resolved Hide resolved
SrcFlowTags string
DstFlowTags string
FlowTags string
DstBgpAsPath string
DstBgpCommunity string
SrcBgpAsPath string
Ipv6SrcAddr []byte
Ipv6SrcRoutePrefix []byte
Customs []Custom
Ipv6DstRoutePrefix []byte
Ipv6DstAddr []byte
Ipv6SrcNextHop []byte
Ipv6DstNextHop []byte
Comment on lines +21 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to move all of these variable length items to the end of the struct? If they come earlier, it seems like they can go against struct padding as opposed to the fixed length fields?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at https://cs.opensource.google/go/x/tools/+/refs/tags/v0.9.3:go/analysis/passes/fieldalignment/fieldalignment.go;l=23-49

This analyzer find structs that can be rearranged to use less memory, and provides
a suggested edit with the most compact order.

Note that there are two different diagnostics reported. One checks struct size,
and the other reports "pointer bytes" used. Pointer bytes is how many bytes of the
object that the garbage collector has to potentially scan for pointers, for example:

	struct { uint32; string }

have 16 pointer bytes because the garbage collector has to scan up through the string's
inner pointer.

	struct { string; *uint32 }

has 24 pointer bytes because it has to scan further through the *uint32.

	struct { string; uint32 }

has 8 because it can stop immediately after the string pointer.

Be aware that the most compact order is not always the most efficient.
In rare cases it may cause two variables each updated by its own goroutine
to occupy the same CPU cache line, inducing a form of memory contention
known as "false sharing" that slows down both goroutines.

I guess it's the difference between pointer bytes and struct size. It would probably be good to try out the different options internally via some actual data and see how they compare before bringing it back up here unless it is well known that this current strategy works better. I assume based on the libraries used that it should be good, but if we're doing this optimization we might as well be sure.

InBytes uint64
InPkts uint64
InputPort uint32
IpSize uint32
Ipv4DstAddr uint32
Ipv4SrcAddr uint32
L4DstPort uint32
L4SrcPort uint32
OutputPort uint32
Protocol uint32
SampledPacketSize uint32
SrcAs uint32
SrcGeo uint32
SrcMac uint32 // IGNORED - use SrcEthMac
TcpFlags uint32
Tos uint32
VlanIn uint32
VlanOut uint32
Ipv4NextHop uint32
MplsType uint32
Timestamp int64
DstEthMac uint64
TimestampNano int64
OutBytes uint64
SrcEthMac uint64
OutPkts uint64
SampleRate uint32
DstThirdAsn uint32
MplsType uint32
VlanOut uint32
VlanIn uint32
TcpRetransmit uint32
AppProtocol uint32
SrcFlowTags string
DstFlowTags string
SampleRate uint32
Tos uint32
TcpFlags uint32
SrcMac uint32 // IGNORED - use SrcEthMac
DeviceId uint32
FlowTags string
Timestamp int64
DstBgpAsPath string
DstBgpCommunity string
SrcBgpAsPath string
SrcBgpCommunity string
SrcGeo uint32
SrcAs uint32
SampledPacketSize uint32
Protocol uint32
OutputPort uint32
L4SrcPort uint32
SrcNextHopAs uint32
DstNextHopAs uint32
SrcGeoRegion uint32
DstGeoRegion uint32
SrcGeoCity uint32
DstGeoCity uint32
Big bool
SampleAdj bool
DstAs uint32
DstGeo uint32
Ipv4DstNextHop uint32
Ipv4SrcNextHop uint32
SrcRoutePrefix uint32
DstRoutePrefix uint32
SrcRouteLength uint8
DstRouteLength uint8
DstMac uint32 // IGNORED - use DstEthMac
HeaderLen uint32
SrcSecondAsn uint32
DstSecondAsn uint32
SrcThirdAsn uint32
DstThirdAsn uint32
Ipv6DstAddr []byte
Ipv6SrcAddr []byte
SrcEthMac uint64
DstEthMac uint64
Ipv6SrcNextHop []byte
Ipv6DstNextHop []byte
Ipv6SrcRoutePrefix []byte
Ipv6DstRoutePrefix []byte
Ipv4NextHop uint32
L4DstPort uint32
Ipv4SrcAddr uint32
Ipv4DstAddr uint32
IpSize uint32
InputPort uint32
DstRouteLength uint8
SrcRouteLength uint8
SampleAdj bool
IsMetric bool
Customs []Custom
Big bool
}

type Type int
Expand All @@ -104,20 +104,20 @@ const (
)

type Custom struct {
ID uint32
Type Type
Str string
U8 byte
U16 uint16
U32 uint32
U64 uint64
I8 int8
I16 int16
I32 int32
Type Type
aaronfuj marked this conversation as resolved.
Show resolved Hide resolved
F64 float64
I64 int64
I32 int32
U32 uint32
ID uint32
F32 float32
F64 float64
I16 int16
U16 uint16
Addr [17]byte
I8 int8
U8 byte
}

func New(cflow *Ckflow) Flow {
Expand Down
6 changes: 3 additions & 3 deletions send.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,15 @@ type Sender struct {
agg *agg.Agg
exit chan struct{}
url *url.URL
timeout time.Duration
client *api.Client
sample int
ticker *time.Ticker
workers sync.WaitGroup
dns chan []byte
Device *api.Device
Errors chan<- error
Metrics *metrics.Metrics
workers sync.WaitGroup
timeout time.Duration
sample int
}

func newSender(url *url.URL, timeout time.Duration) *Sender {
Expand Down