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

refine settings javascript_max_memory_bytes and correct set hard limit #718

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

yokofly
Copy link
Collaborator

@yokofly yokofly commented May 14, 2024

fix #716
so this PR do 3 things:

  1. increase the default javascript_max_memory_bytes to 200MByte from 100MByte
  2. Correct v8 memory hard limit, previous we do not set a value, and will hit v8 abort if the uda try to alloc too many memory.
  3. Correctly apply settings javascript_max_memory_bytes. previous we pass a current_thread::context, now we use query_context.

for 3:
in latest proton server below query will passed, because the value is always 100MByte instead we want 2MByte.

CREATE STREAM IF NOT EXISTS 99010_udf_types(`f32` float);

CREATE AGGREGATE FUNCTION test_sec_large_99010(value float32) RETURNS float32 LANGUAGE JAVASCRIPT AS $$
                  {
                    initialize: function() {
                       this.max = -1.0;
                       this.sec = -1.0
                    },
                    process: function(values) {
                      for (let i = 0; i < values.length; i++) {
                        if (values[i] > this.max) {
                          this.sec = this.max;
                          this.max = values[i]
                        }
                        if (values[i] < this.max && values[i] > this.sec)
                          this.sec = values[i];
                      }
                    }  ..... hide other function
              $$;

select sleep(1) FORMAT Null;
insert into 99010_udf_types(f32) values(2.0);   
select sleep(1) FORMAT Null;

Before this PR: Below CTE query will always use the default of 100MB. If the memory usage exceeds 100MB, trying to modify settings javascript_max_memory_bytes=2 to 200MB will not change the outcome.
Now: An error will be reported.

with acc as (select test_sec_large_99010(f32) as res from table(99010_udf_types) settings javascript_max_memory_bytes=2) select min(res) as re from acc;

qijun-niu-timeplus and others added 4 commits December 10, 2024 17:23
* v8 heap limit less than javascript_max_memory_bytes

* fix
* switch to function arg passed context, instead thread context

* switch the default v8 heap soft limit from 100MB to 200 MB

* move log to ctor. output like
```
2024.03.01 23:52:39.428662 [ 200546 ] {0b772a75-c715-4ffc-8a9f-c980c835842c} <Information> JavaScriptAggregateFunction: udf name=test_sec_large, javascript_max_memory_bytes=23
```
@yokofly yokofly force-pushed the bugfix/issue-716-udf-memory-limit branch from 6605d30 to 855596d Compare December 11, 2024 05:07
@yokofly yokofly changed the title edit the v8 heap hard limit. refine settings javascript_max_memory_bytes and correct set hard limit Dec 11, 2024
@yokofly yokofly marked this pull request as ready for review December 11, 2024 05:25
src/V8/Utils.h Outdated Show resolved Hide resolved
src/V8/Utils.cpp Outdated Show resolved Hide resolved
@yokofly yokofly marked this pull request as draft December 12, 2024 04:47
@yokofly yokofly marked this pull request as ready for review December 12, 2024 14:34
@yokofly yokofly requested a review from chenziliang December 15, 2024 14:17
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.

udf/v8: memory hard limit is not controllable.
3 participants