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

Introduce FixedAppender #8789

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

John-Colvin
Copy link
Contributor

@John-Colvin John-Colvin commented Jul 30, 2023

This provides an appender that doesn't hold its data in an indirection which needs allocating, the lack of which reduces overhead substantially for situations where the amount of actual work done with the appender is small (e.g. the number of elements appended is small). For maybe 95% of use-cases, the user does not want to pass an appender around by value, in fact it's hard to see why that would be desirable, so we take advantage of that and create a non-copyable FixedAppender which can store all its data "in-place".

I intend to spread its use around Phobos initially, for testing the interface, then expose it publicly later (so we can defer arguing about the name too much for now plz).

A separate attempt was made to improve Appender by deferring the allocation to the first copy using copy-constructors, but this did not go well due to problems with inout, const etc. but also that had the consequence of causing an allocation in a somewhat unexpected - and different - place. Hence, it was abandoned for now in favour of this.

The following is a diff to std.json, which when applied with the test code below it & a big json file (taken from https://github.com/json-iterator/test-data/blob/master/large-file.json) led to a reduction in runtime from 400-430 ms to around 350 ± 8 ms

@@ -1133,7 +1133,7 @@ if (isSomeFiniteCharInputRange!T)
         import std.uni : isSurrogateHi, isSurrogateLo;
         import std.utf : encode, decode;
 
-        auto str = appender!string();
+        auto str = fixedAppender!string();
 
     Next:
         switch (peekChar())
@@ -1301,7 +1301,7 @@ if (isSomeFiniteCharInputRange!T)
 
             case '0': .. case '9':
             case '-':
-                auto number = appender!string();
+                auto number = fixedAppender!string();
                 bool isFloat, isNegative;
 
                 void readInteger()
@@ -1498,7 +1498,7 @@ Set the $(LREF JSONOptions.specialFloatLiterals) flag is set in `options` to enc
 */
 string toJSON(const ref JSONValue root, in bool pretty = false, in JSONOptions options = JSONOptions.none) @safe
 {
-    auto json = appender!string();
+    auto json = fixedAppender!string();
     toJSON(json, root, pretty, options);
     return json.data;
 }

test_json.d

import std.json;
import std.file;
import std.datetime.stopwatch : StopWatch;
import std.stdio : writeln;

void main() {
    auto input = readText("large-file.json");
    StopWatch sw;
    sw.start;
    auto output = input.parseJSON.toString;
    sw.stop;
    sw.peek.writeln;
    write("output.json", output);
}

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @John-Colvin! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8789"

@John-Colvin John-Colvin force-pushed the fixed_appender branch 2 times, most recently from 29bd96d to d08bba3 Compare July 30, 2023 16:50
Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

This looks good to me, will wait for others to review though.

@John-Colvin
Copy link
Contributor Author

I think that test failure might be legit, looking in to it now.

Also considering a different approach using a prefix allocator... let's see...

@John-Colvin
Copy link
Contributor Author

@deadalnix suggests "inPlaceAppender" as a name, idk... better than "fixed".

@thewilsonator thewilsonator added the Salvage This PR needs work, but we want to keep it and it can be salvaged. label Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Salvage This PR needs work, but we want to keep it and it can be salvaged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants