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

Reduce string allocations for literals [skip CI] #1590

Merged
merged 27 commits into from
Dec 6, 2023

Conversation

bbannier
Copy link
Member

@bbannier bbannier commented Nov 10, 2023

With this PR the benchmark from #1589 shows identical performance for both cases for me. Overall runtime seems to improve as well.

Before:

$ hyperfine --export-markdown results.md -w3 'cat test-data.txt | ./build/bin/spicy-driver t.hlto' 'cat test-data.txt | ./build/bin/spicy-driver t-compiled-with-realpath.hlto'
Benchmark 1: cat test-data.txt | ./build/bin/spicy-driver t.hlto
  Time (mean ± σ):      1.713 s ±  0.018 s    [User: 1.699 s, System: 0.031 s]
  Range (min … max):    1.695 s …  1.738 s    10 runs
 
Benchmark 2: cat test-data.txt | ./build/bin/spicy-driver t-compiled-with-realpath.hlto
  Time (mean ± σ):      2.166 s ±  0.014 s    [User: 2.141 s, System: 0.041 s]
  Range (min … max):    2.147 s …  2.191 s    10 runs
 
Summary
  cat test-data.txt | ./build/bin/spicy-driver t.hlto ran
    1.26 ± 0.02 times faster than cat test-data.txt | ./build/bin/spicy-driver t-compiled-with-realpath.hlto

After:

$ hyperfine --export-markdown results.md -w3 'cat test-data.txt | ./build/bin/spicy-driver t.hlto' 'cat test-data.txt | ./build/bin/spicy-driver t-compiled-with-realpath.hlto'
Benchmark 1: cat test-data.txt | ./build/bin/spicy-driver t.hlto
  Time (mean ± σ):      1.218 s ±  0.008 s    [User: 1.204 s, System: 0.029 s]
  Range (min … max):    1.209 s …  1.232 s    10 runs
 
Benchmark 2: cat test-data.txt | ./build/bin/spicy-driver t-compiled-with-realpath.hlto
  Time (mean ± σ):      1.219 s ±  0.005 s    [User: 1.202 s, System: 0.031 s]
  Range (min … max):    1.215 s …  1.231 s    10 runs
 
Summary
  cat test-data.txt | ./build/bin/spicy-driver t.hlto ran
    1.00 ± 0.01 times faster than cat test-data.txt | ./build/bin/spicy-driver t-compiled-with-realpath.hlto

Closes #1589.
Closes #1591.

@bbannier bbannier self-assigned this Nov 10, 2023
@bbannier
Copy link
Member Author

The remaining work here is dealing with the fallout from type mismatches from const char* vs std::string. This is likely exactly why we introduced the explicit typing. I picked const char* instead of std::string_view since const char* can implicitly convert to std::string, but maybe introducing a dedicated type would give us more control.

@bbannier bbannier force-pushed the topic/bbannier/issue-1589 branch from 10c4f54 to a014630 Compare November 10, 2023 11:13
@bbannier bbannier force-pushed the topic/bbannier/issue-1589 branch from a014630 to 2a2251e Compare November 17, 2023 22:42
@bbannier
Copy link
Member Author

I slightly reworked this since while we can emit C++ literals for generated (effectively static) strings user-specified literals should still be emitted as mutable strings (e.g., so users can call mutating functions on them). The way I now detect the difference is to check whether a literal has a location.

@bbannier bbannier marked this pull request as ready for review November 18, 2023 10:26
@bbannier bbannier force-pushed the topic/bbannier/issue-1589 branch from bff2b0e to f0b6fb3 Compare November 20, 2023 09:14
@bbannier bbannier requested a review from rsmmr November 20, 2023 11:18
@bbannier
Copy link
Member Author

I added a couple of other perf cleanups to this PR, please let me know if I should pull them into their own PR.

Copy link
Member

@rsmmr rsmmr left a comment

Choose a reason for hiding this comment

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

Main comment is that "magic" part: not a fan. Rest looks good.

@bbannier bbannier force-pushed the topic/bbannier/issue-1589 branch from f0b6fb3 to 5fad5b4 Compare November 20, 2023 15:01
rsmmr
rsmmr previously approved these changes Nov 20, 2023
@bbannier bbannier changed the title Reduce string allocations for literals Reduce string allocations for literals [skip CI] Nov 22, 2023
@bbannier bbannier force-pushed the topic/bbannier/issue-1589 branch 2 times, most recently from 43880db to 5315a54 Compare December 1, 2023 19:43
rsmmr
rsmmr previously approved these changes Dec 6, 2023
@@ -1,5 +1,7 @@
// Copyright (c) 2020-2023 by the Zeek Project. See LICENSE for details.

#include "spicy/rt/sink.h"
Copy link
Member

Choose a reason for hiding this comment

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

#include <spicy/rt/sink.h>

Copy link
Member Author

Choose a reason for hiding this comment

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

Even though we still do not do this consistently, I would really like to include this with #include "..." to make clear that this is the header related to this .cc file. With that we e.g., get it to sort before other headers which helps make sure that the header is self-contained.

Copy link
Member

Choose a reason for hiding this comment

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

I think we had this discussion a few times already. :-) I won't fight it, I just don't like the inconsistency of doing it randomly for a few implementation files, but not most others.

When waiting for input we pass down strings for a possible error message
and the triggering location. In generated code these are always
literals.

With this patch we do not take them as owning strings, but instead as
views into existing strings to minimize allocations. In the case of
error messages the created low-level exception objects already had used
string_views, so this also aligns the APIs.

Closes #1589.
When emitting literals for HILTI strings (string ctors) we would
previously explicitly force creation of `std::string`. This was almost
always an unnecessary pessimisation over emitting string literals since
even if their C++ uses expected `std::string` string literals can
convert to this type implicitly; at the same time it made it impossible
to make effective use of APIs accepting `std::string_view`.

With this patch we now emit C++ string literals for HILTI string
literals.
@bbannier bbannier force-pushed the topic/bbannier/issue-1589 branch from e696c78 to cb43a7a Compare December 6, 2023 11:01
@bbannier bbannier merged commit 66984e2 into main Dec 6, 2023
@bbannier bbannier deleted the topic/bbannier/issue-1589 branch December 6, 2023 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants