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

Make script cache and compilation limits per context #50152

Closed
jdconrad opened this issue Dec 12, 2019 · 5 comments
Closed

Make script cache and compilation limits per context #50152

jdconrad opened this issue Dec 12, 2019 · 5 comments
Assignees
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement Team:Core/Infra Meta label for core/infra team v8.0.0-alpha1

Comments

@jdconrad
Copy link
Contributor

Currently, we have setting to apply a max for the script cache and settings to determine the max rate of compilation. These apply to all script contexts. We should change the settings to be per script context as each context has different needs.

Relates to #49763

@jdconrad jdconrad added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache v8.0.0 labels Dec 12, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Scripting)

@stu-elastic
Copy link
Contributor

stu-elastic commented Dec 19, 2019

In ScriptService
SCRIPT_CACHE_SIZE_SETTING and SCRIPT_MAX_COMPILATIONS_RATE, SCRIPT_CACHE_EXPIRE_SETTING needs to be per context as well as Cache<CacheKey, Object> cache; along with scriptMetrics, which is where they are being tracked.

SCRIPT_MAX_COMPILATIONS_RATE needs to be able to be unlimited.

In compile we can sychronize on the context rather than this. Can add arbitrary private object used for locking.

Idea: encapsulate these into a ContextCompiler, move all compile logic into here.

@stu-elastic
Copy link
Contributor

stu-elastic commented Feb 18, 2020

Code staging:

  1. General context cache
  2. Add settings for context caches. Make them dynamically updatable. Default to general compiler. Ability to transition to context caches without restart.
  3. Add defaults for each context
  4. Add unlimited compile rate settings
  5. Default to per context settings and deprecate old general settings
  6. Update stats API

Behavior:
If general settings, use general compiler and deprecate log
If no general settings use context specific
If general settings and context specific, error
-> cluster update settings for context specific and general then context specific rejected

stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Feb 19, 2020
Phase 1 of adding compilation limits per context.
* Refactor compilation and caching into separate class, ContextCompiler
  which will be used per context.
* Disable compilation limit for tests.
* Add script.max_compilations_rate = "unlimited" setting which disables
  compilation limits.

Refs: elastic#50152
stu-elastic added a commit that referenced this issue Feb 20, 2020
Phase 1 of adding compilation limits per context.
* Refactor rate limiting and caching into separate class, 
  `ScriptCache`,  which will be used per context.
* Disable compilation limit for certain tests.

Refs: #50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Feb 21, 2020
Phase 1 of adding compilation limits per context.
* Refactor rate limiting and caching into separate class,
  `ScriptCache`,  which will be used per context.
* Disable compilation limit for certain tests.

Refs: elastic#50152
stu-elastic added a commit that referenced this issue Feb 21, 2020
Phase 1 of adding compilation limits per context.
* Refactor rate limiting and caching into separate class,
  `ScriptCache`,  which will be used per context.
* Disable compilation limit for certain tests.

Backport of 0866031
Refs: #50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Feb 24, 2020
Phase 1 of adding compilation limits per context.
* Refactor compilation and caching into separate class, ContextCompiler
  which will be used per context.
* Disable compilation limit for tests.
* Add script.max_compilations_rate = "unlimited" setting which disables
  compilation limits.

Refs: elastic#50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Feb 26, 2020
Phase 1 of adding compilation limits per context.
* Refactor compilation and caching into separate class, ContextCompiler
  which will be used per context.
* Disable compilation limit for tests.
* Add script.max_compilations_rate = "unlimited" setting which disables
  compilation limits.

Refs: elastic#50152
@stu-elastic
Copy link
Contributor

Should we disable caching all together if a user sets an unlimited compilation rate?

stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Mar 10, 2020
* Adds per context settings:
  `script.context.${CONTEXT}.cache_max_size` ~
  `script.cache.max_size`

  `script.context.${CONTEXT}.cache_expire` ~
  `script.cache.expire`

  `script.context.${CONTEXT}.max_compilations_rate` ~
  `script.max_compilations_rate`

* Context cache is used if:
  `script.max_compilations_rate=use-context`.  This
  value is dynamically updatable, so users can
  switch back to the general cache if desired.

* Settings for context caches take the first value
  that applies:
  1) Context specific settings if set, eg
     `script.context.ingest.cache_max_size`
  2) Correlated general setting is set to the non-default
     value, eg `script.cache.max_size`
  3) Context default

The reason for 2's inclusion is to allow an easy
transition for users who've customized their general
cache settings.

Using the general cache settings for the context caches
results in higher effective settings, since they are
multiplied accross the number of contexts.  So a general
cache max size of 200 will become 200 * # of contexts.
However, this behavior it will avoid users snapping to a
value that is too low for them.

Refs: elastic#50152
stu-elastic added a commit that referenced this issue Mar 18, 2020
* Adds per context settings:
  `script.context.${CONTEXT}.cache_max_size` ~
  `script.cache.max_size`

  `script.context.${CONTEXT}.cache_expire` ~
  `script.cache.expire`

  `script.context.${CONTEXT}.max_compilations_rate` ~
  `script.max_compilations_rate`

* Context cache is used if:
  `script.max_compilations_rate=use-context`.  This
  value is dynamically updatable, so users can
  switch back to the general cache if desired.

* Settings for context caches take the first value 
  that applies:
  1) Context specific settings if set, eg
     `script.context.ingest.cache_max_size`
  2) Correlated general setting is set to the non-default 
     value, eg `script.cache.max_size`
  3) Context default

The reason for 2's inclusion is to allow an easy
transition for users who've customized their general
cache settings.

Using the general cache settings for the context caches
results in higher effective settings, since they are 
multiplied across the number of contexts.  So a general
cache max size of 200 will become 200 * # of contexts.
However, this behavior it will avoid users snapping to a
value that is too low for them.


Refs: #50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Mar 18, 2020
* Adds per context settings:
  `script.context.${CONTEXT}.cache_max_size` ~
  `script.cache.max_size`

  `script.context.${CONTEXT}.cache_expire` ~
  `script.cache.expire`

  `script.context.${CONTEXT}.max_compilations_rate` ~
  `script.max_compilations_rate`

* Context cache is used if:
  `script.max_compilations_rate=use-context`.  This
  value is dynamically updatable, so users can
  switch back to the general cache if desired.

* Settings for context caches take the first value
  that applies:
  1) Context specific settings if set, eg
     `script.context.ingest.cache_max_size`
  2) Correlated general setting is set to the non-default
     value, eg `script.cache.max_size`
  3) Context default

The reason for 2's inclusion is to allow an easy
transition for users who've customized their general
cache settings.

Using the general cache settings for the context caches
results in higher effective settings, since they are
multiplied across the number of contexts.  So a general
cache max size of 200 will become 200 * # of contexts.
However, this behavior it will avoid users snapping to a
value that is too low for them.

Refs: elastic#50152
stu-elastic added a commit that referenced this issue Mar 18, 2020
* Adds per context settings:
  `script.context.${CONTEXT}.cache_max_size` ~
  `script.cache.max_size`

  `script.context.${CONTEXT}.cache_expire` ~
  `script.cache.expire`

  `script.context.${CONTEXT}.max_compilations_rate` ~
  `script.max_compilations_rate`

* Context cache is used if:
  `script.max_compilations_rate=use-context`.  This
  value is dynamically updatable, so users can
  switch back to the general cache if desired.

* Settings for context caches take the first value
  that applies:
  1) Context specific settings if set, eg
     `script.context.ingest.cache_max_size`
  2) Correlated general setting is set to the non-default
     value, eg `script.cache.max_size`
  3) Context default

The reason for 2's inclusion is to allow an easy
transition for users who've customized their general
cache settings.

Using the general cache settings for the context caches
results in higher effective settings, since they are
multiplied across the number of contexts.  So a general
cache max size of 200 will become 200 * # of contexts.
However, this behavior it will avoid users snapping to a
value that is too low for them.

Backport of: #52855
Refs: #50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Mar 18, 2020
* Adds ability for contexts to specify their own defaults.
* Context defaults are applied if no context-specific or
  general setting exists.
* See 070ea7e for settings keys.

* Increases the per-context default for the `ingest` context.
* Cache size is doubled, 200 compared to default of 100
* Cache expiration is unchanged at no expiration
* Cache max compilation is quintupled, 375/5m instead of 75/5m

Refs: elastic#50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Mar 18, 2020
* Adds "unlimited" compilation rate for context script caches
* `script.context.${CONTEXT}.max_compilations_rate` = `unlimited`
  disables compilation rate limiting for `${CONTEXT}`'s script
  cache

Refs: elastic#50152
stu-elastic added a commit that referenced this issue Mar 20, 2020
* Adds "unlimited" compilation rate for context script caches
* `script.context.${CONTEXT}.max_compilations_rate` = `unlimited`
  disables compilation rate limiting for `${CONTEXT}`'s script
  cache

Refs: #50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Mar 20, 2020
* Adds "unlimited" compilation rate for context script caches
* `script.context.${CONTEXT}.max_compilations_rate` = `unlimited`
  disables compilation rate limiting for `${CONTEXT}`'s script
  cache

Refs: elastic#50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jun 26, 2020
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jun 26, 2020
@stu-elastic
Copy link
Contributor

stu-elastic commented Jun 29, 2020

Having script and script_cache in _node/stats is redundant. Here's the value of _node/stats in 7.x:

      "script" : {
        "compilations" : 172,
        "cache_evictions" : 72,
        "compilation_limit_triggered" : 0
      },

Here's it in 8.x right now:

      "script_cache": {                                                      
        "sum": {                      
          "compilations": 1,                                                 
          "cache_evictions": 0,                                              
          "compilation_limit_triggered": 0                                   
        },                            
        "contexts": [                                                        
          {                           
            "context": "aggregation_selector",                               
            "compilations": 0,                                               
            "cache_evictions": 0,                                            
            "compilation_limit_triggered": 0                                 
          },                                  

We will remove script_cache in 8 and hoist sum to the top level. Here's what we'll have:

      "script": {                          
        "compilations": 1,
        "cache_evictions": 0,
        "compilation_limit_triggered": 0,
        "contexts":[
          {
            "context": "aggregation_selector",                               
            "compilations": 0,                                               
            "cache_evictions": 0,                                            
            "compilation_limit_triggered": 0                                 
          },                                  

This keeps the format the same between 7 and 8 so we can easily drop script_cache. Also the sum is unnecessary indirection.

stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jun 29, 2020
Removed settings:
 * `script.cache.max_size`
 * `script.cache.expire`
 * `script.max_compilations_rate`

Updated `_nodes/stats`:
 * Remove `script_cache`
 * Update `script` in `_node/stats` to include stats per context:
```
      "script": {
        "compilations": 1,
        "cache_evictions": 0,
        "compilation_limit_triggered": 0,
        "contexts":[
          {
            "context": "aggregation_selector",
            "compilations": 0,
            "cache_evictions": 0,
            "compilation_limit_triggered": 0
          },

```

Refs: elastic#50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jul 1, 2020
 * Remove `script_cache`
 * Update `script` in `_node/stats` to include stats per context:

```
      "script": {
        "compilations": 1,
        "cache_evictions": 0,
        "compilation_limit_triggered": 0,
        "contexts":[
          {
            "context": "aggregation_selector",
            "compilations": 0,
            "cache_evictions": 0,
            "compilation_limit_triggered": 0
          },

```
Refs: elastic#50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jul 8, 2020
Removed settings:
 * `script.cache.max_size`
 * `script.cache.expire`
 * `script.max_compilations_rate`

Updated `_nodes/stats`:
 * Remove `script_cache`
 * Update `script` in `_node/stats` to include stats per context:

```
      "script": {
        "compilations": 1,
        "cache_evictions": 0,
        "compilation_limit_triggered": 0,
        "contexts":[
          {
            "context": "aggregation_selector",
            "compilations": 0,
            "cache_evictions": 0,
            "compilation_limit_triggered": 0
          },

```

Refs: elastic#50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jul 8, 2020
Updated `_nodes/stats`:
 * Remove `script_cache`
 * Update `script` in `_node/stats` to include stats per context:
```

      "script": {

        "compilations": 1,

        "cache_evictions": 0,

        "compilation_limit_triggered": 0,
        "contexts":[
          {
            "context": "aggregation_selector",
            "compilations": 0,
            "cache_evictions": 0,
            "compilation_limit_triggered": 0
          },

```

Refs: elastic#50152
Backport: elastic#59205
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jul 8, 2020
Removed settings:
 * `script.cache.max_size`
 * `script.cache.expire`
 * `script.max_compilations_rate`

Refs: elastic#50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jul 9, 2020
Removed settings:
 * `script.cache.max_size`
 * `script.cache.expire`
 * `script.max_compilations_rate`

Refs: elastic#50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jul 9, 2020
Updated `_nodes/stats`:
 * Remove `script_cache`
 * Update `script` in `_node/stats` to include stats per context:

```
      "script": {
        "compilations": 1,
        "cache_evictions": 0,
        "compilation_limit_triggered": 0,
        "contexts":[
          {
            "context": "aggregation_selector",
            "compilations": 0,
            "cache_evictions": 0,
            "compilation_limit_triggered": 0
          },

```

Refs: elastic#50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jul 9, 2020
Updated `_nodes/stats`:
 * Update `script` in `_node/stats` to include stats per context:

```
      "script": {
        "compilations": 1,
        "cache_evictions": 0,
        "compilation_limit_triggered": 0,
        "contexts":[
          {
            "context": "aggregation_selector",
            "compilations": 0,
            "cache_evictions": 0,
            "compilation_limit_triggered": 0
          },

```

Refs: elastic#50152
Backport: elastic#59625
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jul 9, 2020
* `ingest` and `processor_conditional` default to unlimited compilation rate

Refs: elastic#50152
stu-elastic added a commit to stu-elastic/elasticsearch that referenced this issue Jul 9, 2020
* `ingest` and `processor_conditional` default to unlimited compilation rate

Refs: elastic#50152
stu-elastic added a commit that referenced this issue Jul 9, 2020
Removed settings:
 * `script.cache.max_size`
 * `script.cache.expire`
 * `script.max_compilations_rate`

Refs: #50152
stu-elastic added a commit that referenced this issue Jul 9, 2020
Updated `_nodes/stats`:
 * Remove `script_cache`
 * Update `script` in `_node/stats` to include stats per context:

```
      "script": {
        "compilations": 1,
        "cache_evictions": 0,
        "compilation_limit_triggered": 0,
        "contexts":[
          {
            "context": "aggregation_selector",
            "compilations": 0,
            "cache_evictions": 0,
            "compilation_limit_triggered": 0
          },

```

Refs: #50152
stu-elastic added a commit that referenced this issue Jul 9, 2020
Updated `_nodes/stats`:
 * Update `script` in `_node/stats` to include stats per context:

```
      "script": {
        "compilations": 1,
        "cache_evictions": 0,
        "compilation_limit_triggered": 0,
        "contexts":[
          {
            "context": "aggregation_selector",
            "compilations": 0,
            "cache_evictions": 0,
            "compilation_limit_triggered": 0
          },

```

Refs: #50152
Backport: #59625
stu-elastic added a commit that referenced this issue Jul 9, 2020
* `ingest` and `processor_conditional` default to unlimited compilation rate

Refs: #50152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement Team:Core/Infra Meta label for core/infra team v8.0.0-alpha1
Projects
None yet
Development

No branches or pull requests

5 participants