Skip to content

Reduce the minimum size for packed arrays from 8 to 2 #5

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

Closed
wants to merge 16 commits into from

Conversation

TysonAndre
Copy link
Owner

@TysonAndre TysonAndre commented Sep 29, 2019

There are probably ways this could be optimized (e.g. use a limit of 1 or 0 instead, better places to compute minimums).
For now, I'm aiming to see if this will work correctly.

sapi/phpdbg/tests/watch_006.phpt fails after recent changes to allow sizes that aren't powers of 2.

@TysonAndre
Copy link
Owner Author

TysonAndre commented Sep 29, 2019

This seems like it causes a noticeable drop in memory usage. (12% in one application. I haven't tried others, and expect it to vary)

E.g. for phan self-analysis, phan --print-memory-usage-summary reports 460MB/592MB for a regular build, and 425MB/522MB (end memory vs max memory)

  • EDIT: It decreased to 387MB/475MB after removing the power of 2 constraint.

@TysonAndre TysonAndre force-pushed the hash-minsize-demo branch 3 times, most recently from ff32c24 to 1b866d8 Compare September 29, 2019 21:48
@TysonAndre
Copy link
Owner Author

@nikic - Do you have any thoughts on this?

  • How would I benchmark this change to see if this made code faster or slower? I only see Zend/bench.php, but that wouldn't be representative of large applications. (My laptop is unsuitable for benchmarking - times are all over the place after intel speculation mitigations)
  • Should I submit this for comments to php-src as-is?
  • Should there be separate helpers for initializing packed arrays without rounding up the size to a power of 2?

@TysonAndre TysonAndre force-pushed the hash-minsize-demo branch 2 times, most recently from 69053a5 to ae85ede Compare October 5, 2019 16:34
@TysonAndre TysonAndre closed this Oct 7, 2019
TysonAndre pushed a commit that referenced this pull request Apr 25, 2021
When encountering the following SSA graph:

    BB1:
    #2.T1 [string] = COALESCE #1.CV0($str) [null, string] BB2

    BB2:
    #5.T1 [string] = QM_ASSIGN string("")

    BB3:
    #7.X1 [string] = Phi(#2.X1 [string], #5.X1 [string])
    FREE #7.T1 [string]

We would currently determine that #7, #5 are dead, and eliminate
the FREE and QM_ASSIGN. However, we cannot eliminate #2, as
COALESCE is also responsible for control flow.

Fix this my marking all non-CV phis as live to start with. This
can be relaxed to check the kind of the source instruction, but
I couldn't immediately come up with a case where it would be
useful.
TysonAndre pushed a commit that referenced this pull request May 31, 2021
Part of generated opcodes for $foo are:

  ...
  BB1:
  0002 INIT_FCALL 1 96 string("foo")
  0003 #5.V1 [rcn, object (instanceof A)] = FETCH_THIS
  0004 SEND_REF #5.V1 [rcn, object (instanceof A)] 1
  0005 DO_UCALL

Updates in functions zend_jit_fetch_this() and zend_jit_load_this() are
made to support FETCH_THIS opcode.

One new path is covered in function zend_jit_send_ref()  by SEND_REF
opcode.
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.

1 participant