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

Export huffman loopkup init table #9

Merged
merged 1 commit into from Jan 11, 2020
Merged

Conversation

@nicolas-grekas
Copy link
Contributor

nicolas-grekas commented Jan 9, 2020

Right now, huffman lookup init takes 150ms on my machine on the cli (85ms with opcache).

This table has been generated with the patch below applied to Symfony VarExporter's Exporter class, then this just before returning in HuffmanLookupInit:

ksort($encodingAccess);
foreach ($encodingAccess as $k => $v) {
    ksort($encodingAccess[$k], SORT_STRING);
}
file_put_contents(
    __DIR__.'/huffman-lookup.php',
    "<?php\n\nreturn ".\Symfony\Component\VarExporter\VarExporter::export($encodingAccess).";\n"
);
--- a/src/Symfony/Component/VarExporter/Internal/Exporter.php
+++ b/src/Symfony/Component/VarExporter/Internal/Exporter.php
@@ -212,6 +212,8 @@ class Exporter
         $subIndent = $indent.'    ';
 
         if (\is_string($value)) {
+            return '"'.str_replace('%', '\x', rawurlencode($value)).'"';
+
             $code = sprintf("'%s'", addcslashes($value, "'\\"));
 
             $code = preg_replace_callback('/([\0\r\n]++)(.)/', function ($m) use ($subIndent) {
@@ -240,6 +242,10 @@ class Exporter
         }
 
         if (\is_array($value)) {
+            if ([0, 1] === array_keys($value) && !\is_array($value[0]) && !\is_array($value[1])) {
+                return '['.self::export($value[0]).', '.self::export($value[1]).']';
+            }
+
             $j = -1;
             $code = '';
             foreach ($value as $k => $v) {
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:init branch 3 times, most recently from e84ae13 to 8206eb6 Jan 9, 2020
@kelunik

This comment has been minimized.

Copy link
Member

kelunik commented Jan 9, 2020

We're definitely interested in optimizing the lookup table. Time spent can be significant, but also the 33 MB RAM are rather huge, especially when compared to the nghttp2 implementation.

I haven't really looked into the lookup table construction, but maybe we can optimize that, too, maybe even with a decreased decoding performance in favor of a better init performance.

Maybe we should also base the used variant on the SAPI being used and whether opcache is available.

@kelunik

This comment has been minimized.

Copy link
Member

kelunik commented Jan 9, 2020

As I just saw it on your other profile, did you have xdebug loaded while benchmarking?

@trowski

This comment has been minimized.

Copy link
Member

trowski commented Jan 9, 2020

Yes, I'm surprised that RAM usage increased for CLI, but decreased for webserver, or am I misunderstanding something?

@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 9, 2020

RAM usage increases because PHP needs memory to parse the new big file.
It decreases on the web because it leverages opcache's shared memory better I suppose.
But I know how to improve this even further, update coming :)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:init branch from 8206eb6 to 8dacdbb Jan 9, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 9, 2020

Et voilà the new benchmark:
image

I confirm xdebug was disabled.

The right side is all zeros because the call was so fast that it has been pruned by Blackfire.

The reason for the memory going to zero is that the table is now a purely static array, which means it stays in opcache's shared memory only.

PR ready :)

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:init branch 4 times, most recently from 96e0400 to dec7835 Jan 9, 2020
src/Internal/HPackNative.php Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:init branch 8 times, most recently from dc1923e to 5a34955 Jan 10, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Contributor Author

nicolas-grekas commented Jan 10, 2020

Note that we could compact each line in huffman-lookup.php into single strings, replacing the myriad of small arrays.
But now that these arrays are in shared memory, this would provide nothing more.

PR ready.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:init branch from 5a34955 to c4822e6 Jan 10, 2020
@kelunik kelunik self-assigned this Jan 11, 2020
@kelunik kelunik merged commit c4822e6 into amphp:master Jan 11, 2020
1 of 2 checks passed
1 of 2 checks passed
coverage/coveralls Coverage decreased (-0.5%) to 59.412%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kelunik

This comment has been minimized.

Copy link
Member

kelunik commented Jan 11, 2020

Thanks! We can look into further optimizations in another PR if it's necessary.

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:init branch Jan 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.