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

Fix cost inflation with Xdebug 3 #141

Closed
wants to merge 1 commit into from

Conversation

TRowbotham
Copy link

This should fix #132, though I don't have an Xdebug 2 profile to compare to. Based on the findings in #132 Xdebug 2 reported costs in microseconds, but Xdebug 3 now reports those costs in nanoseconds. This looks at the events header, which explicitly declares the time unit used for the costs, if defined, and dynamically adjusts the divisor based on the declared time unit.

This seemed like the easiest fix. Since Xdebug 3 outputs much larger numbers due to the change to nanoseconds, the preprocessor is probably at greater risk of integer overflow, but that is a pre-existing problem.

@TRowbotham
Copy link
Author

After thinking about this more and looking at some of my recent profiles, its pretty clear the whole integer overflow thing is a very big problem with the change to nanoseconds; at least for the summary line, I haven't spent the time check the costs of individual functions.

A recent profile generated by Xdebug 3 from one of my projects (profiling only took 1.26 min) ends up producing a summary line of:

$ grep -I 'summary:' /tmp/cachegrind.out.1618_1622494851.330687
summary: 9047770910 62830704

9,047,770,910 is clearly outside the range of a 32-bit number and it ends up being truncated when packed:

$ psysh
Psy Shell v0.10.8 (PHP 7.4.3 — cli) by Justin Hileman
>>> $num = 9047770910;
=> 9047770910
>>> $p = pack('V*', $num);
=> "\x1E\x07J\e"
>>> unpack('V*', $p)
=> [
     1 => 457836318,
   ]

A more proper fix would be to check for the events header in the pre-processors, then convert the numbers to something smaller, such as microseconds, before packing. This would probably require a bump in the file format version, so as to invalidate any webgrind generated files prior to normalizing the stored costs number. Though, it may not be sufficient should any of the individual costs exceed the size of a signed 32-bit int before normalization.

The only other options I can think of here is to switch to using a big int library and packing the numbers as strings, or possibly dropping support for running webgrind on 32-bit versions of PHP and changing the packing format to "P". Both of which would require a bump in the file format version.

I'll leave it up to you if you want to take this patch as an interim fix.

@alpha0010
Copy link
Collaborator

Thanks for your contribution. Decided to go with a simplified version of this; the full solution would need to be done in the preprocesser, so avoiding complexity in the reader (which would later be removed).

@alpha0010 alpha0010 closed this Sep 17, 2021
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.

Xdebug 3.0 upgrade appears to exponentially increase the total time (cost).
2 participants