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

Introduce new way of create object in script #1303

Merged
merged 3 commits into from
Jan 18, 2024
Merged

Conversation

ksh8281
Copy link
Contributor

@ksh8281 ksh8281 commented Jan 16, 2024

No description provided.

Signed-off-by: Seonghyun Kim <sh8281.kim@samsung.com>
@ksh8281 ksh8281 changed the title Work66 @ksh8281 Introduce new way of create object in script Jan 16, 2024
@ksh8281 ksh8281 changed the title @ksh8281 Introduce new way of create object in script Introduce new way of create object in script Jan 16, 2024
* Prepare the property and key list before object creation
* The new way reduce size of object structure with transition

Signed-off-by: Seonghyun Kim <sh8281.kim@samsung.com>
Comment on lines +3014 to +3017
NEVER_INLINE void InterpreterSlowPath::createArrayOperation(ExecutionState& state, CreateArray* code, ByteCodeBlock* byteCodeBlock, Value* registerFile)
{
registerFile[code->m_registerIndex] = new ArrayObject(state, (uint64_t)code->m_length);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add a function for creating Array object here?
This method seems quite simple, so it might be better to embed this function body into the interpreter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, calling GC_MALLOC is quite complicated, so inlining the new expression into the interpreter is not very efficient.
It's only a small effort to reduce the size of the interpreter function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh,, I see

Comment on lines 59 to 62
if (m_properties.size() >= ESCARGOT_OBJECT_STRUCTURE_TRANSITION_MODE_MAX_SIZE) {
size_t objectCreationDataIndex = SIZE_MAX;
size_t initCodePosition = codeBlock->currentCodeSize();
if (m_properties.size()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if statement could be removed since m_properties has always elements in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! this if statement was needed for testing.
I am going to remove it

Comment on lines 134 to 137
if (m_properties.size()) {
codeBlock->peekCode<CreateObjectPrepare>(initCodePosition)->m_propertyReserveSize = propertyIndex;
context->giveUpRegister();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if statement could be removed too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! this if statement was needed for testing.
I am going to remove it

Object* obj = registerFile[code->m_registerIndex].asObject();
ObjectStructureItemTightVector properties;
properties.reset(data->m_properties.takeBuffer(), data->m_values.size());
obj->m_structure = ObjectStructure::create(state.context(), std::move(properties));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of allocating a temporal ObjectStructureItemTightVector(properties) to copy the data->m_properties here,
What about defining data->m_properties as ObjectStructureItemTightVector to just move the data directly?
Is this not possible because properties of the same name could be initialized together ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ObjectExpression not always have immutable count of properties. ex) spread element, duplicated key
But TightVector needs GC_MALLOC every expand of property
That's why I use Vector for data->m_properties

* It should support methods and spread element

Signed-off-by: Seonghyun Kim <sh8281.kim@samsung.com>
Copy link
Contributor

@clover2123 clover2123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@clover2123 clover2123 merged commit bd95de3 into Samsung:master Jan 18, 2024
27 checks passed
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.

2 participants