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

Switch to micmania1's config implementation #6477

Closed
4 tasks done
sminnee opened this issue Jan 11, 2017 · 14 comments
Closed
4 tasks done

Switch to micmania1's config implementation #6477

sminnee opened this issue Jan 11, 2017 · 14 comments

Comments

@sminnee
Copy link
Member

sminnee commented Jan 11, 2017

Acceptance criteria

  • Config uses silverstripe/config module
  • Hard-coded configs can be easily built as a wrapper around an array
  • Config performance is faster

Ingo to talk to Michael about this.

PRS:

@chillu
Copy link
Member

chillu commented Jan 12, 2017

@micmania1 FYI, Sam has assigned this to 4.0.0-alpha5 at the moment, which is planned for around a month from today. Are there any documented blockers or TODOs? Existing discussion: https://groups.google.com/forum/#!searchin/silverstripe-dev/config%7Csort:relevance/silverstripe-dev/q5khashNiuY/0Q7zkKwwAwAJ

@micmania1
Copy link
Contributor

@chillu The new implementation relies on PSR-6 cache interface which I believe is still up for discussion?

@tractorcow tractorcow self-assigned this Feb 9, 2017
@tractorcow
Copy link
Contributor

tractorcow commented Feb 9, 2017

I'm going to keep a list of todos based on my review (as I go along):

Caching

We can probably formalise this when we substitute the current cache system. We will need to determine the best invalidation / regeneration strategy. Is this explicitly on flush? What about if mtime on any source yml file is modified?

Another question I have is whether we really want to cache on a per-key basis, rather than caching the entire config itself. psr-6 caching generates objects for individual cached items, which would mean a new object is created for every config value referenced.

My reasoning is that it's impossible to re-load the config JUST for a single key; You need to load all sources in one go, in case a single key is contained within them.

The current implementation simply returns null if a cache is missed, rather than falling back to an uncached lookup, which means we aren't really treating it as a cache, as much as a config database.

Reproduce lookup options (e.g. inheritance, first_set)

We could probably move the lookup behaviour to a level above the core config (which can be pure key-value pairs). E.g. Some config lookup helper that applies class-hierarchy and applied extensions on top of the config.

That way these extra functions aren't a part of the config itself, they are just extra tools the framework uses to lookup config.

Support nested config collections

I suggest flipping the class construction around to something similar to what @sminnee
proposed in the above discussion.

new CachedConfig(
   new CascadedConfig([
     new YamlConfig(...),
     new PrivateStatticConfig(...),
   ]),
   new PSR6CacheHandler(...)
 );

This would make ConfigCollections responsible for invoking ->transform() on their config sources as-needed.

Cacheable user-modified config

Just brainstorming some ideas to increase performance where user-modified code provides updated config.

I see these as a great substitute for having to invoke Config::inst()->set(). I imagine a UserModifiedConfig store which can be provided a list of callbacks which are conditionally invoked when the cache is regenerated. Alternatively, the non-cached ConfigCollection could just store these on ->set() and invoke the callback on ->get().

E.g. instead of

$extraFields = $this->buildFields();
Config::inst()->update(SiteTree::class, 'db', $extraFields);

The cacheable version of this would be

Config::inst()->update(SiteTree::class, 'db', function() {
	return $this->buildFields();
});

The callback would then be invoked if either the cache is expired, or if currently flushing.

Config merging outside of YML

This implementation appears to only merge config between YML files; If I have a config array defined in both yml and my class, values will be overwritten, not merged. The same issue applies when I wish to merge config with Config::inst()->update. Do we need to split Config::inst()->merge( and Config::inst()->set(?

Without this, for instance, it would be impossible to add extra 'allowed_actions` to a class, only to overwrite with new actions.

Substitute for DataExtension::get_extra_config

Similar to my comments regarding Config::inst()->update(), we probably need another config source that allows user-code to provide config for custom classes, but in a cacheable manner.

Potentially this could be rolled into the lazy-evaluation of config I noted above.

Alternatively, this functionality could be ported into the "lookup options" helper I noted above, with the cost that it will be done in a non-cacheable manner (on a level above the config and its cache). It may NEED to be done in this way, as extensions themselves are applied via config (similar issue to the environment via config paradox earlier discussed).

I use this in fluent, so I'd like to ensure it's still possible to do in 4.0. :) https://github.com/tractorcow/silverstripe-fluent/blob/master/code/extensions/FluentExtension.php#L303

@tractorcow
Copy link
Contributor

Please see #6611

@tractorcow
Copy link
Contributor

tractorcow commented Feb 15, 2017

WIP as at:

This vs current master will be benchmarked:

  • Time to build cache (cold)
  • Time to boot from cached config (warm)
  • Time to query SiteTree::config()->get('db')
  • Time to query SiteTree::config()->get('db', Config::EXCLUDE_EXTRA_SOURCES)
  • Time to modify SiteTree::config()->update('db') and re-requery this value.

@tractorcow
Copy link
Contributor

@tractorcow
Copy link
Contributor

tractorcow commented Feb 16, 2017

Results:

1 Build config with cold cache 100 times (flush=all)

rewrite: 39.7 seconds
master: 20.4 seconds

2 Build config from cache 100 times (not flushed)

rewrite: 5.3 seconds
master: 5.0 seconds

3 Query SiteTree::config()->get('db') 10000 times (uninherited, w/ extensions)

rewrite: 0.56s (was 1.7 before adding additional memory cache)
master: 0.71s

4 Query SiteTree::config()->get('db') 10000 times (uninherited, no extensions)

rewrite: 0.53s
mater: 0.53s

5 Modify SiteTree.db and re-query 10000 times (uninherited, w/ extensions)

rewrite: 0.57s
master: 0.81s

6 Modify SiteTree.db and re-query 10000 times (uninherited, no extensions)

rewrite: 0.56s
master: 0.57s

@tractorcow
Copy link
Contributor

So new config did better in test 5, but much worse in test 3. I think there's something wrong with my caching for middleware-applied values (that can be fixed).

@tractorcow
Copy link
Contributor

ok, applying a non-persistant cache on top of CachedConfigCollection seems to have improved the performance a bit. :)

@tractorcow
Copy link
Contributor

tractorcow commented Feb 16, 2017

Actually, that extra cached bumped the performance to faster than the master config. I've adjusted results for test 3.

@tractorcow
Copy link
Contributor

tractorcow commented Feb 16, 2017

Repeating test 2 reveals that there is quite a bit of range in my testing results. I might increase the repetitions and re-do it. I'll try 1000 instead of 100.

@tractorcow
Copy link
Contributor

tractorcow commented Feb 16, 2017

Bump up to 1000 for the below tests:

1 Build config with cold cache 1000 times (flush=all)

rewrite: 5m58.268s
master: 3m18.600s

2 Build config from cache 1000 times (not flushed)

rewrite: 54.619s
master: 53.549s

@tractorcow
Copy link
Contributor

tractorcow commented Feb 16, 2017

Conclusion: Generally faster in day to day usage, slightly slower to load from cache (2% slower), but a lot slower to build the cache on the first hit.

@sminnee
Copy link
Member Author

sminnee commented Feb 16, 2017

@tractorcow I think that performance situation is good enough to merge, and we can look at further optimisations as we see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants