Lazy initialization of charset maps #166

toddtreece · 2023-12-20T03:45:21Z

Why

The initialization of the charsetMaps is on average ~5% of heap inuse_space on an idle Grafana instance. It would be ideal if we could avoid this memory overhead until the related code is used.

Related Grafana PR: grafana/grafana#79729

What

Switch to lazy initialization of charsetMaps.

codecov-commenter · 2023-12-20T03:55:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5821c4e) 74.41% compared to head (bce011a) 74.34%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #166      +/-   ##
==========================================
- Coverage   74.41%   74.34%   -0.07%     
==========================================
  Files          31       31              
  Lines        6253     6253              
==========================================
- Hits         4653     4649       -4     
- Misses       1322     1324       +2     
- Partials      278      280       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

shueybubbles · 2023-12-20T14:34:55Z

thx for opening a PR!

shueybubbles · 2023-12-20T14:36:43Z

internal/cp/charset.go

 type charsetMap struct {
 	sb [256]rune    // single byte runes, -1 for a double byte character lead byte
 	db map[int]rune // double byte runes
 }

+// lazy initialization of all charset maps
+func initCharsetMaps() {
+	initOnce.Do(func() {


my guess is most apps only use 1 or 2 collations. Maybe it'd be worthwhile to lazy init them individually instead of all at once.

good call. i'll make that change

this approach is ok but you could also merge the init into a function that returns the variable.
Something like

case 30,31,32,33,34: return getcp437()

ideally there'd be no named variable that someone could reference without it first being initialized but I don't know if it's feasible with this model without some unnecessarily exotic dependency inversion.

updated from init -> get in 6644c35

toddtreece · 2023-12-20T18:39:12Z

@microsoft-github-policy-service agree company="Grafana Labs"

toddtreece · 2024-02-14T16:08:45Z

@shueybubbles @apoorvdeshmukh thanks for the review! i just wanted to check on the status of this one and see if there was anything preventing the merge/release.

toddtreece mentioned this pull request Dec 20, 2023

Perf: Lazy initialization of charsetmaps in go-mssqldb grafana/grafana#79729

Merged

3 tasks

shueybubbles assigned apoorvdeshmukh and stuartpa Dec 20, 2023

shueybubbles reviewed Dec 20, 2023

View reviewed changes

toddtreece force-pushed the main branch from f6c7c81 to 00eaca5 Compare December 20, 2023 18:44

shueybubbles approved these changes Dec 20, 2023

View reviewed changes

toddtreece requested a review from shueybubbles December 20, 2023 19:27

shueybubbles approved these changes Dec 20, 2023

View reviewed changes

shueybubbles unassigned apoorvdeshmukh and stuartpa Dec 20, 2023

shueybubbles requested review from apoorvdeshmukh and stuartpa December 20, 2023 23:07

apoorvdeshmukh approved these changes Jan 23, 2024

View reviewed changes

toddtreece added 3 commits February 12, 2024 10:53

lazy initialization of charset maps

04f0ad4

initialize each charsetmap separately

11c614c

switch from init to get

bce011a

toddtreece force-pushed the main branch from 6644c35 to bce011a Compare February 12, 2024 16:00

shueybubbles merged commit b650081 into microsoft:main Feb 14, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy initialization of charset maps #166

Lazy initialization of charset maps #166

toddtreece commented Dec 20, 2023

codecov-commenter commented Dec 20, 2023 •

edited

Loading

shueybubbles commented Dec 20, 2023

shueybubbles Dec 20, 2023

toddtreece Dec 20, 2023

shueybubbles Dec 20, 2023

toddtreece Dec 20, 2023

toddtreece commented Dec 20, 2023

toddtreece commented Feb 14, 2024

Lazy initialization of charset maps #166

Lazy initialization of charset maps #166

Conversation

toddtreece commented Dec 20, 2023

Why

What

codecov-commenter commented Dec 20, 2023 • edited Loading

Codecov Report

shueybubbles commented Dec 20, 2023

shueybubbles Dec 20, 2023

Choose a reason for hiding this comment

toddtreece Dec 20, 2023

Choose a reason for hiding this comment

shueybubbles Dec 20, 2023

Choose a reason for hiding this comment

toddtreece Dec 20, 2023

Choose a reason for hiding this comment

toddtreece commented Dec 20, 2023

toddtreece commented Feb 14, 2024

codecov-commenter commented Dec 20, 2023 •

edited

Loading