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.

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

Improves memory usage of ingest path. #118

Merged
merged 4 commits into from Jul 5, 2022

Conversation

cyriltovena
Copy link
Contributor

@cyriltovena cyriltovena commented Jul 4, 2022

❯ benchstat before.txt after.txt                                                                                                                                         
name                                       old time/op    new time/op    delta
_Table_Insert_10Rows_10Iters_10Writers-16     457ms ±11%     550ms ± 8%  +20.56%  (p=0.008 n=5+5)

name                                       old alloc/op   new alloc/op   delta
_Table_Insert_10Rows_10Iters_10Writers-16     577MB ±25%     169MB ±33%  -70.78%  (p=0.008 n=5+5)

name                                       old allocs/op  new allocs/op  delta
_Table_Insert_10Rows_10Iters_10Writers-16     1.06M ±36%     0.55M ±50%  -47.59%  (p=0.032 n=5+5)

Not yet sure why we're 20% slower, could be the sync.Map, nothing terrible is showing in cpu profiling, but never the less, the memory usage is way better.

See #94

}

func (s *Schema) GetWriter(w io.Writer, dynamicColumns map[string][]string) (*PooledWriter, error) {
key := serializeDynamicColumns(dynamicColumns)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically I need to also sort each suffix too at the risk of the cache not working, now sure if that's required or not that could be a big to do each time.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure exactly if this is what you mean but keys are sorted, and values for keys are sorted, so there should be no different keys with the same dynamic columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if values were ! Thanks for clarifying

@cyriltovena
Copy link
Contributor Author

Latest commit seems to have improve CPU situation.

name                                       old time/op    new time/op    delta
_Table_Insert_10Rows_10Iters_10Writers-16     479ms ±12%     523ms ± 5%     ~     (p=0.095 n=5+5)

name                                       old alloc/op   new alloc/op   delta
_Table_Insert_10Rows_10Iters_10Writers-16     545MB ±19%     152MB ±31%  -72.01%  (p=0.008 n=5+5)

name                                       old allocs/op  new allocs/op  delta
_Table_Insert_10Rows_10Iters_10Writers-16     1.12M ±59%     0.37M ±15%  -67.42%  (p=0.016 n=5+4)

Comment on lines +370 to +375
var rowBufPool = &sync.Pool{
New: func() interface{} {
return make([]parquet.Row, 64) // Random guess.
},
}

Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to have this initialized when calling NewSchema rather than having it globally in the package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is actually shared across all schemas.

@metalmatze
Copy link
Member

Very nice! 🎉

Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

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

I have to admit, it took me a couple of re-reads to understand the logic, but this approach is pretty nifty, we don't have to do any unnecessary accesses to the sync map, which is really good!

@brancz brancz merged commit f949727 into polarsignals:main Jul 5, 2022
@cyriltovena
Copy link
Contributor Author

I used the sync.Map because it performs very well when you write only once, better than a RWMutex.

The Map type is optimized for two common use cases: (1) when the entry for a given key is only ever written once but read many times, as in caches that only grow, or (2)

My only wish would be to avoid the empty pool creation required by the LoadOrStore.

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.

None yet

3 participants