Skip to content

Add Memory support#1

Merged
jarstelfox merged 6 commits into
masterfrom
memory-or-time
Nov 6, 2023
Merged

Add Memory support#1
jarstelfox merged 6 commits into
masterfrom
memory-or-time

Conversation

@danielbeardsley
Copy link
Copy Markdown
Member

Support retrieving and using the memory information that XDebug records.

Adding "memory" as cost format option was the easiest way to get it done.

Easier to read / maintian this way.
Xdebug has supported memory for a while, but this tool hadn't been
updated to use it.

Here we add a new "costFormat" option of "bytes of memory".
We pass this through to the pre-processor to have it read the memory
cost field instead of the time cost field.
Just like the PHP pre-processor, we read the memory if costFormat ==
'bytes'.
Comment thread library/Preprocessor.php
static function parse($inFile, $outFile)
static function parse($inFile, $outFile, $readMemory = false)
{
$costPattern = $readMemory ? "%*d %d" : "%d";
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could you explain the reason for this pattern?
The %*d is skipping the number correct?
What number is in the second / third positions?

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.

Yes, every "cost line" looks like this:
4 936 160 == {line number} {time cost in usec} {memory cost in bytes}

When we want to read memory, we effectively need to skip the time cost and read the last number (which is normally unread).

Copy link
Copy Markdown

@andyg0808 andyg0808 left a comment

Choose a reason for hiding this comment

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

CR 👍
Looks reasonable.

Comment thread library/FileHandler.php
Comment on lines +163 to +170
$readMemory = $costFormat === 'bytes';
$memorySuffix = $readMemory ? ".memory" : "";
$prepFile = Webgrind_Config::storageDir().$file.$memorySuffix.Webgrind_Config::$preprocessedSuffix;
try {
$r = new Webgrind_Reader($prepFile, $costFormat);
} catch (Exception $e) {
// Preprocessed file does not exist or other error
Webgrind_Preprocessor::parse(Webgrind_Config::xdebugOutputDir().$file, $prepFile);
Webgrind_Preprocessor::parse(Webgrind_Config::xdebugOutputDir().$file, $prepFile, $readMemory);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah! Cool, you are creating two pre-processed files (one for memory, one for CPU)

Copy link
Copy Markdown

@jarstelfox jarstelfox left a comment

Choose a reason for hiding this comment

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

CR 🌵

@kthaler kthaler self-assigned this Nov 3, 2023
@kthaler kthaler added the QAing Under inspection for quality assurance label Nov 3, 2023
@kthaler
Copy link
Copy Markdown

kthaler commented Nov 3, 2023

QA 💎 - Webgrind now receives and presents memory information.
deploy_block 🦖 - I'm noticing some serious discrepancies between kcachegrind (qcachegrind for me on M1) and Webgrind. For example:

Screenshot 2023-11-03 at 1 53 22 PM

If this is alright, then please disregard the deploy block.

@kthaler kthaler removed the QAing Under inspection for quality assurance label Nov 3, 2023
@danielbeardsley
Copy link
Copy Markdown
Member Author

I made a minimal example and the data is coming out correct in both tools for me. Actually, it appears that kcachegrind has a bug.

<?

function main() {
   $x[] = useMemory(100);
   $x[] = useMemory(1000);
   $x[] = useMemory(10000);
   $x[] = useMemory(100000);
   $x[] = useMemory(1000000);
   $x[] = useMemory(10000000);
}

function useMemory(int $bytes) {
   return str_repeat("x", $bytes);
}

main();

Note that kcachegrind says that main() has 0 bytes of inclusive cost. This is obviously false, so I'm not sure what's going on. Maybe it's cause I named in main like the app entrypoint? I'll try a more complex example.

image

@danielbeardsley
Copy link
Copy Markdown
Member Author

Nah, it seems like it's a bug in kcachegrind

Here's a slightly more complex example:

function main() {
   $x[] = use10KB();
   $x[] = use1MB();
}

function use1MB() {
   $overhead = useMemory(100);
   return useMemory(10000000);
}

function use10KB() {
   $overhead = useMemory(100);
   return useMemory(10000);
}

function useMemory(int $bytes) {
   return str_repeat("x", $bytes);
}

main();

image

@danielbeardsley
Copy link
Copy Markdown
Member Author

@jarstelfox recommended I change "main" to something else. No dice, kcachegrind still shows the bug:

image

@danielbeardsley
Copy link
Copy Markdown
Member Author

dev_block 👍 the binary pre-processor is producing the wrong results.

@jarstelfox
Copy link
Copy Markdown

jarstelfox commented Nov 6, 2023

I do wish there was some test suite here.
You should open this up on the upstream 💙

Welp, your examples give me confidence that this is working as expected.

looks like you found a bug

Some other pull upgraded jQuery and that caused this jQuery plugin to
crash. Stubbing this "browser" object will allow expressions like
"$.browser.ie" to return undefined instead of throwing.
Previously, the tablesorter was detecting numeric values with thousands
separators as "text" and sorting them incorrectly.

I added thousands separators for bytes values, so they are easier to
read at a glance as the numbers get long but the sorter hadn't been
parsing them well.

Note: I'm not sure why the "Cost" column was set to "sorter: false" on
the subtables, it had still been sorting but it had been falling back to
auto-detecting the sort type.
@danielbeardsley
Copy link
Copy Markdown
Member Author

un_dev_block 👍 The outputs are the same, it was a sorting type auto-detection issue.

Copy link
Copy Markdown

@jarstelfox jarstelfox left a comment

Choose a reason for hiding this comment

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

CR 🌵

Copy link
Copy Markdown

@andyg0808 andyg0808 left a comment

Choose a reason for hiding this comment

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

CR 👍

@kthaler kthaler added QAing Under inspection for quality assurance and removed QAing Under inspection for quality assurance labels Nov 6, 2023
@kthaler
Copy link
Copy Markdown

kthaler commented Nov 6, 2023

QA 💎 - The bytes of memory cost option in webgrind is displaying correct values.

un_deploy_block 🦕 - Fixed the problem with my qcachegrind and it is now showing the same/similar values as webgrind.

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.

4 participants