Skip to content

Use plain-array for static_variables#1707

Closed
nikic wants to merge 1 commit intophp:masterfrom
laruence:static-vars
Closed

Use plain-array for static_variables#1707
nikic wants to merge 1 commit intophp:masterfrom
laruence:static-vars

Conversation

@nikic
Copy link
Copy Markdown
Member

@nikic nikic commented Dec 31, 2015

No description provided.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This naming is confusing... maybe static_var_names and static_vars?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree, just meant to be similar with op_array->vars.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we really need to keep an array of static variable names?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We might use indirect addressing into vars array by using u2 inside static_variables?

@nikic
Copy link
Copy Markdown
Member Author

nikic commented Dec 31, 2015

A general comment: Previously the static variables table could be shared between multiple functions based on refcount. At least until the first static variable is bound by executing the function.

Was this sharing important for performance? Will it be a problem if it's no longer present?

@laruence
Copy link
Copy Markdown
Member

yeah, we need duplicated the static_variables when op_array is copied from shared memory out to php proceses, I was thinking of introduce IS_TYPE_IMMUTABLE which is marked in static_variables[0].u2 . however , if the function is executed enough times, it will be faster (if the hash lookup time beat memroy duplication time).

@laruence laruence force-pushed the static-vars branch 3 times, most recently from b918cb3 to 2028829 Compare January 2, 2016 09:45
@laruence laruence force-pushed the static-vars branch 2 times, most recently from cfab746 to 1d183a8 Compare January 3, 2016 02:15
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In general, "val" may be a part of "var" and destroying "var" before incrementing "val", may cause problems. (may be I'm wrong).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

val always should have a minimum refcount of 1 as long as it is referenced in static_variables array. So it shouldn't be an issue.

@laruence
Copy link
Copy Markdown
Member

part of this has been committed as 6579e48 , close this

@laruence laruence closed this Jan 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants