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

don't allocate config inside With() #48

Open
mrsinham opened this issue Jun 22, 2021 · 1 comment
Open

don't allocate config inside With() #48

mrsinham opened this issue Jun 22, 2021 · 1 comment
Assignees

Comments

@mrsinham
Copy link

Describe the feature

We would be glad to not instanciate a pointer to a config inside every call to With() (so calls to New() by extension).

Motivation

Recently we had an "Out of memory" error on our process because the config included inside every With() ate up to 40GB of RAM.

Thank you a lot and we love your library. Kudos.

@Akaame
Copy link
Contributor

Akaame commented Aug 21, 2021

config included inside every With() ate up to 40GB of RAM.

You should have called this method at least several million times for that to happen. That is impressive.

What did you do as a hotfix? One way out of this would be instantiating DefaultConfig at the start of your program. Could you please share the solution on your end?

From a users' perspective there is not much of a point in leaving DefaultConfig nil then checking if it is nil only to instantiate it with an actual default configuration. Semantically this does not much make sense. Ideally my proposed solution would be

DefaultConfig := &Config{
	WeekStartDay: WeekStartDay,
	TimeFormats:  TimeFormats,
}

// get rid of nil check logic for DefaultConfig in With impl

in main.go

Edit:

This issue seems unsolvable with the strategy above. Making DefaultConfig "default" and then changing the body of With as follows:

// old
func With(t time.Time) *Now {
	config := DefaultConfig
	if config == nil {
		config = &Config{
			WeekStartDay: WeekStartDay,
			TimeFormats:  TimeFormats,
		}
	}

	return &Now{Time: t, Config: config}
}

// new
func With(t time.Time) *Now {
	return &Now{Time: t, Config: DefaultConfig}
}

sadly breaks testing because most of the testing cases depend on WeekStartDay and TimeFormats being manipulated on the fly. So "changing library configuration by changing these global variables" is broken after the fix above (since we do not get a new instance of config we are not aware of these global config being changed). Changing such behaviour will result in API breakage as most of the users of this library depend on manipulating WeekStartDay to "Monday" or appending their own TimeFormats.

Solutions for consumers of the library:

  • Avoid using free With and use config.With instead
  • If you want to continue using With assign DefaultConfig at the start of your program.

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

No branches or pull requests

3 participants