Add removing unused parameter bytecode process to parser by kwonjeomsim · Pull Request #1543 · Samsung/escargot

kwonjeomsim · 2026-02-27T01:41:59Z

This PR adds some processes to parser to pass creation of bytecode for unused function parameters to optimize memory usage.

To implement this feature, we need to add some variables in ASTScopeContext and InterpretedCodeBlock which checking parameter using. And actual check process is executed when closing function scope.

When eval function and arguments is in function body, all parameters in this scope(all parent scopes too for arguments) are considered as used because of complexity of implementation.

Also, if statement starting at 3806 in esprima.cpp needs to care test/vendortest/v8/test/mjsunit/es6/regress/regress-4395.js test.

I test this feature with web-tooling-benchmark to watch peak memory usage, but GC was running irregularly so result shows good performance sometime, or not. Differance is about 10 to 100 KB up or down.

Here is some result of benchmark tests:

buble.js : 582,367 -> 582,344 (-23 KB)
terser.js : 582,387 -> 582,427 (+40 KB)
acorn.js : 582,404 -> 582,412 (+8 KB)
babel-minify.js : 582,480 -> 582,377 (-103 KB)

ksh8281 · 2026-02-27T01:50:36Z

src/parser/CodeBlock.h

 #endif

+#ifndef ESCARGOT_DEBUGGER
+    uint16_t m_parameterUsed : 16;


Please add comment about meaning
like 0xFFFF means the function have many parameters(>16)
....

github-actions · 2026-02-27T11:31:55Z

src/parser/esprima_cpp/esprima.cpp

+                for (size_t j = 0; j < ret->m_childBlockScopes.size(); j++) {
+                    ASTBlockContext* block = ret->m_childBlockScopes[j];
+                    if (VectorUtil::findInVector(block->m_usingNames, paramName) != VectorUtil::invalidIndex) {
+                        scopeCtx->m_parameterUsed |= (1 << i);


The scopeCtx->m_parameterUsed |= (1 << i); line may shift 1 by i bits, which can overflow if i is greater than the bit width of m_parameterUsed, leading to undefined behavior. Ensure i is within bounds or use a wider type or bitset.

clover2123 · 2026-02-28T11:36:29Z

src/parser/ast/ASTContext.h

+#ifndef ESCARGOT_DEBUGGER
+    uint16_t m_parameterUsed : 16; // 0xFFFF means all parameters are used or function has more than 16 parameters

+    ASTScopeContext *m_parentScope;


This approach searches every use case of parameter in bottom-up method, so this additional m_parentScope is required.
But have you tried top-down method instead?
It seems possible to search from top to bottom without this additional info

I tried that already, and it is possible. But that way uses almost equal memory size compared to this way, and gets more complicated code because of adding process which dealing with arguments object.
Additionaly, top-down search implementation itself is more complicated than bottom-up search.
So I think bottom-up search is better than top-down.

@hyukwoo-park
I rewrited to top-down method, and it looks better than my thought.
Please check again!

github-actions · 2026-03-02T17:42:43Z

src/parser/esprima_cpp/esprima.cpp

+    void setParameterUsedValue(ASTScopeContext* scopeCtx)
+    {
+        while (scopeCtx) {
+            for (size_t i = 0; i < this->currentScopeContext->m_parameters.size(); i++) {


this->currentScopeContext->m_parameters is used in the loop header of setParameterUsedValue, but the function receives scopeCtx. As a result, when setParameterUsedValue is called recursively on child scopes, the loop still iterates over the original currentScopeContext's parameters, leaving child scopes' parameters unprocessed and potentially mis-marking used parameters. This logic flaw can lead to incorrect parameter usage detection in the parser. The minimal fix is to replace this->currentScopeContext with scopeCtx in the loop header and all subsequent accesses inside the function.

github-actions · 2026-03-02T17:42:45Z

src/parser/esprima_cpp/esprima.cpp

    }

+#ifndef ESCARGOT_DEBUGGER
+    void setParameterUsedValue(ASTScopeContext* scopeCtx)


Use the function argument scopeCtx consistently inside setParameterUsedValue instead of this->currentScopeContext. The current loop accesses this->currentScopeContext->m_parameters and this->currentScopeContext->m_parameterUsed, which can be confusing because the function receives scopeCtx. Switching to scopeCtx clarifies that the function operates on the passed context and prevents accidental reliance on the outer currentScopeContext. This small change improves readability and maintainability without altering the overall logic. It also reduces the risk of subtle bugs when the function is called recursively on child scopes.

src/parser/esprima_cpp/esprima.cpp

clover2123 · 2026-03-03T04:47:57Z

src/parser/esprima_cpp/esprima.cpp

    }

+#ifndef ESCARGOT_DEBUGGER
+    void setParameterUsedValue(ASTScopeContext* scopeCtx)


minor review) IMO setParameterUsedValue is not intuitive, what about another name such as checkParameterUsage or others?

clover2123 · 2026-03-03T04:51:36Z

src/parser/esprima_cpp/esprima.cpp

+#endif
+#ifndef ESCARGOT_DEBUGGER
+        if (UNLIKELY(paramNames.size() > 16)) {
+            this->currentScopeContext->m_parameterUsed = 0xFFFF;


0xFFFF might be confusing, so It would be better to use macro variable like below (just an example)

#define DISABLE_PARAMETER_REMOVE 0xFFFF

clover2123 · 2026-03-03T06:40:30Z

src/parser/esprima_cpp/esprima.cpp

+                if (this->currentScopeContext->m_parameterUsed == DISABLE_PARAM_CHECK) {
+                    break;
+                }


Please move this check code out of the outermost while statement (with UNLIKELY macro), and it seems possible to return directly inside this check statement

clover2123 · 2026-03-03T06:47:28Z

src/parser/esprima_cpp/esprima.cpp

+                    break;
+                }
+
+                AtomicString name = this->currentScopeContext->m_parameters[i];


This line could be improved as below (using a reference)

// parameter list is fixed, so using a reference out of the while statement to shorten the code AtomicStringTightVector& parameters = this->currentScopeContext->m_parameters; while (...) { ... AtomicString& name = parameters[i];

And in this approach, the outermost while could be removed

clover2123 · 2026-03-03T06:55:28Z

src/parser/esprima_cpp/esprima.cpp

+            if (scopeCtx->firstChild()) {
+                checkUsedParameters(scopeCtx->firstChild());
+            }
+
+            if (scopeCtx != this->currentScopeContext) {
+                scopeCtx = scopeCtx->nextSibling();
+            } else {
+                break;
+            }
+        }


This part could be improved(refactored) below

size_t childCount = scopeCtx->childCount(); if (childCount > 0) { ASTScopeContext* childScope = ScopeCtx->firstChild(); for (size_t i = 0; i < childCount; i++) { checkUsedParameters(childScope); childScope = childScope->nextSibling(); } }

I think that this approach is better for maintenance.

clover2123 · 2026-03-03T07:05:59Z

src/parser/esprima_cpp/esprima.cpp

    ASTScopeContext* popScopeContext(ASTScopeContext* lastPushedScopeContext)
    {
+#ifndef ESCARGOT_DEBUGGER
+        checkUsedParameters(this->currentScopeContext);


checkUsedParameters is invoked at each popScopeContext; this means that parameter check is done after each scan of the function completes.
That is, at this moment, we can know whether this function uses eval or has more than 16 parameters.
So, we can check the above cases just before invoking checkUsedParameters here.

github-actions · 2026-03-03T11:11:30Z

src/parser/esprima_cpp/esprima.cpp

+            for (size_t j = 0; j < scopeCtx->m_childBlockScopes.size(); j++) {
+                ASTBlockContext& blockCtx = *scopeCtx->m_childBlockScopes[j];
+                if (VectorUtil::findInVector(blockCtx.m_usingNames, name) != VectorUtil::invalidIndex) {
+                    this->currentScopeContext->m_parameterUsed |= (1 << i);


The expression 1 << i in this->currentScopeContext->m_parameterUsed |= (1 << i); performs a left shift by the loop index i. If i is greater than or equal to the bit width of m_parameterUsed, the shift is undefined and can corrupt the bitmask. Cast the shift to a wider type or guard against i exceeding the width of m_parameterUsed before setting the bit.

github-actions · 2026-03-03T12:45:54Z

src/parser/esprima_cpp/esprima.cpp

+                if (VectorUtil::findInVector(blockCtx.m_usingNames, name) != VectorUtil::invalidIndex) {
+                    this->currentScopeContext->m_parameterUsed |= (1 << i);
+                    break;
+                } else if (scopeCtx->m_parameterUsed == DISABLE_PARAM_CHECK && VectorUtil::findInVector(blockCtx.m_usingNames, stringArguments) != VectorUtil::invalidIndex) {


The condition checks scopeCtx->m_parameterUsed == DISABLE_PARAM_CHECK but the assignment later sets this->currentScopeContext->m_parameterUsed = DISABLE_PARAM_CHECK. This mismatch means the disable flag may never be detected, causing incorrect parameter usage checks. Change the condition to test this->currentScopeContext->m_parameterUsed instead of scopeCtx->m_parameterUsed to ensure the flag is correctly evaluated.

github-actions · 2026-03-04T12:30:08Z

src/parser/ast/ASTContext.h

 #endif
+#ifndef ESCARGOT_DEBUGGER
+        , m_hasStringArguments(false)
+        , m_parameterUsed(0)


Remove the duplicate initialization of m_parameterUsed(0) inside the second #ifndef ESCARGOT_DEBUGGER block. The struct already initializes m_parameterUsed in the first conditional block, so the second occurrence causes a duplicate member initialization error when ESCARGOT_DEBUGGER is not defined. Keeping a single initialization ensures the constructor remains well-formed and avoids compilation failures. If the intention was to add a new member, consider renaming or moving the initialization to a separate block.

github-actions · 2026-03-04T12:30:09Z

src/parser/esprima_cpp/esprima.cpp

+                if (VectorUtil::findInVector(blockCtx.m_usingNames, name) != VectorUtil::invalidIndex) {
+                    this->currentScopeContext->m_parameterUsed |= (1 << i);
+                    break;
+                } else if (scopeCtx->m_parameterUsed == DISABLE_PARAM_CHECK) {


The else if now triggers whenever scopeCtx->m_parameterUsed == DISABLE_PARAM_CHECK, causing an early return that skips checking remaining parameters. This can lead to incorrect m_parameterUsed state. Restore the original condition or add a guard for stringArguments.

clover2123 · 2026-03-05T06:23:02Z

src/parser/ast/ASTContext.h

+    bool m_hasStringArguments : 1;
+    uint16_t m_parameterUsed : 16;


Considering padding, please locate each boolean and uint16_t member together with other boolean and 16-bit sized members

clover2123 · 2026-03-05T06:23:24Z

Looks much better 👍

clover2123 · 2026-03-05T13:45:15Z

@ksh8281
Could you review this patch?
In real apps, quite a lot of parameters are defined but not used.
So, we try to search these cases and optimize them by just removing parameter-load bytecodes
If there is another better way, please share your opinion

ksh8281 · 2026-03-06T01:58:09Z

Please add comment about meaning of m_parameterUsed
like 0xFFFF means the function have many parameters(>16)

kwonjeomsim requested review from bbrto21, clover2123 and ksh8281 as code owners February 27, 2026 01:42

ksh8281 reviewed Feb 27, 2026

View reviewed changes

kwonjeomsim force-pushed the optimize branch 2 times, most recently from 3345bfa to 72bba12 Compare February 27, 2026 11:27

github-actions bot reviewed Feb 27, 2026

View reviewed changes

clover2123 reviewed Feb 28, 2026

View reviewed changes

github-actions bot reviewed Mar 2, 2026

View reviewed changes

clover2123 reviewed Mar 3, 2026

View reviewed changes

src/parser/esprima_cpp/esprima.cpp Outdated Show resolved Hide resolved

kwonjeomsim force-pushed the optimize branch from 8b9aad7 to cab7e3e Compare March 3, 2026 03:02

clover2123 reviewed Mar 3, 2026

View reviewed changes

kwonjeomsim force-pushed the optimize branch 2 times, most recently from b28e581 to a07c5f4 Compare March 3, 2026 06:08

clover2123 reviewed Mar 3, 2026

View reviewed changes

kwonjeomsim force-pushed the optimize branch from a07c5f4 to 1e2d5d2 Compare March 3, 2026 11:02

github-actions bot reviewed Mar 3, 2026

View reviewed changes

kwonjeomsim force-pushed the optimize branch from 1e2d5d2 to 2e3efe1 Compare March 3, 2026 12:42

github-actions bot reviewed Mar 3, 2026

View reviewed changes

kwonjeomsim force-pushed the optimize branch from 2e3efe1 to 626c11e Compare March 4, 2026 12:06

github-actions bot reviewed Mar 4, 2026

View reviewed changes

kwonjeomsim force-pushed the optimize branch from 626c11e to 112ec3a Compare March 4, 2026 12:40

clover2123 reviewed Mar 5, 2026

View reviewed changes

kwonjeomsim force-pushed the optimize branch 4 times, most recently from 9d23dc6 to c26915d Compare March 5, 2026 12:27

kwonjeomsim added 9 commits March 6, 2026 11:15

Add parsing process that remove unused function parameter bytecodes

6e9ce7a

Add InterpretedCodeBlock::ParameterUsed info to codecache

a40bdc9

Change unused parameter check point

5c9e811

Change m_parameterUsed to bit operation and Add hashset

eb3ec20

Change AtomicStringMap allocation and deal with parameters over 16

83652c1

Change location of removing unused parameters

900140b

Add codes checking assignment pattern and rest parameter usage

46aa77c

Change location of optimization logic

f2674fe

Change bottom-up to top-down check

281b69e

kwonjeomsim force-pushed the optimize branch from c26915d to 281b69e Compare March 6, 2026 02:15

kwonjeomsim requested a review from ksh8281 March 6, 2026 04:03

ksh8281 approved these changes Mar 8, 2026

View reviewed changes

ksh8281 merged commit f41ec34 into Samsung:master Mar 8, 2026
31 of 32 checks passed

		bool m_hasStringArguments : 1;
		uint16_t m_parameterUsed : 16;

Conversation

kwonjeomsim commented Feb 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Feb 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clover2123 commented Mar 5, 2026

Uh oh!

clover2123 commented Mar 5, 2026

Uh oh!

ksh8281 commented Mar 6, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kwonjeomsim commented Feb 27, 2026 •

edited

Loading