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

Remove amphp/file dependency #78

Closed
ostrolucky opened this issue Dec 26, 2018 · 0 comments · Fixed by #80
Closed

Remove amphp/file dependency #78

ostrolucky opened this issue Dec 26, 2018 · 0 comments · Fixed by #80

Comments

@ostrolucky
Copy link

I'm only using amphp/socket and there is following dependency tree that is useless to me:
amphp/socker -> amphp/file -> amphp/sync, amphp/parallel

So after removing this dependency, my consumers will have to install 3 packages less.

Additionally, replacing it with bytestream increases performance almost 10x!

Index: bench/HostLoaderBench.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- bench/HostLoaderBench.php	(date 1545833204000)
+++ bench/HostLoaderBench.php	(date 1545833204000)
@@ -0,0 +1,17 @@
+<?php
+
+namespace Amp\Dns\Bench;
+
+use Amp\Dns\HostLoader;
+use Amp\Loop;
+
+class HostLoaderBench
+{
+    public function benchHostLoader()
+    {
+        Loop::run(function () {
+            $loader = new HostLoader(__DIR__ . "/../test/data/hosts");
+            yield $loader->loadHosts();
+        });
+    }
+}
Index: lib/HostLoader.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- lib/HostLoader.php	(revision 836b0896b0105298373669754ece8ad8577c0854)
+++ lib/HostLoader.php	(date 1545834232000)
@@ -2,6 +2,7 @@
 
 namespace Amp\Dns;
 
+use Amp\ByteStream\ResourceInputStream;
 use Amp\File;
 use Amp\Promise;
 use Amp\Uri\InvalidDnsNameException;
@@ -26,7 +27,8 @@
             $data = [];
 
             try {
-                $contents = yield File\get($this->path);
+                $resource = new ResourceInputStream(fopen($this->path, 'r'), filesize($this->path));
+                $contents = yield $resource->read();
             } catch (File\FilesystemException $e) {
                 return [];
             }

Without this change:

benchHostLoader               I99 P0 	[μ Mo]/r: 404.861 400.640 (μs) 	[μSD μRSD]/r: 16.964μs 4.19%

1 subjects, 100 iterations, 100 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 376.960 [404.861 400.640] 490.050 (μs)
⅀T: 40,486.090μs μSD/r 16.964μs μRSD/r: 4.190%

With this change:

benchHostLoader               I99 P0 	[μ Mo]/r: 46.553 44.866 (μs) 	[μSD μRSD]/r: 2.447μs 5.26%

1 subjects, 100 iterations, 100 revs, 0 rejects, 0 failures, 0 warnings
(best [mean mode] worst) = 43.520 [46.553 44.866] 54.270 (μs)
⅀T: 4,655.250μs μSD/r 2.447μs μRSD/r: 5.257%
trowski added a commit that referenced this issue Jan 1, 2019
kelunik pushed a commit that referenced this issue Jan 4, 2019
Replaces async loading of hosts and resolver files with blocking reads by default.

Closes #78.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

1 participant