Skip to content

Commit

Permalink
don't use Symbols for strings
Browse files Browse the repository at this point in the history
string expressions by and large do not need the benefits a Symbol gives us,
instead they pollute the symbol table and cause unnecessary overhead for almost
all strings. the one place we can think of that benefits from them (attrpaths
with expressions) extracts the benefit in the parser, which we'll have to touch
anyway when changing ExprString to hold strings.

this gives a sizeable improvement on of 3-5% on all benchmarks we've run.

 before

  nix search --no-eval-cache --offline ../nixpkgs hello
    Time (mean ± σ):      8.844 s ±  0.045 s    [User: 6.750 s, System: 1.663 s]
    Range (min … max):    8.758 s …  8.922 s    20 runs

  nix eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix
    Time (mean ± σ):     367.4 ms ±   3.3 ms    [User: 332.3 ms, System: 35.2 ms]
    Range (min … max):   364.0 ms … 375.2 ms    20 runs

  nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system'
    Time (mean ± σ):      2.810 s ±  0.030 s    [User: 2.517 s, System: 0.225 s]
    Range (min … max):    2.742 s …  2.854 s    20 runs

 after

  nix search --no-eval-cache --offline ../nixpkgs hello
    Time (mean ± σ):      8.533 s ±  0.068 s    [User: 6.485 s, System: 1.642 s]
    Range (min … max):    8.404 s …  8.657 s    20 runs

  nix eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix
    Time (mean ± σ):     347.6 ms ±   3.1 ms    [User: 313.1 ms, System: 34.5 ms]
    Range (min … max):   343.3 ms … 354.6 ms    20 runs

  nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system'
    Time (mean ± σ):      2.709 s ±  0.032 s    [User: 2.414 s, System: 0.232 s]
    Range (min … max):    2.655 s …  2.788 s    20 runs
  • Loading branch information
pennae committed Jan 19, 2022
1 parent 0a77466 commit 9ac836d
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 13 deletions.
4 changes: 2 additions & 2 deletions src/libexpr/nixexpr.hh
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,9 @@ struct ExprFloat : Expr

struct ExprString : Expr
{
Symbol s;
string s;
Value v;
ExprString(const Symbol & s) : s(s) { v.mkString(s); };
ExprString(std::string s) : s(std::move(s)) { v.mkString(this->s.data()); };
COMMON_METHODS
Value * maybeThunk(EvalState & state, Env & env);
};
Expand Down
22 changes: 11 additions & 11 deletions src/libexpr/parser.y
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ static void addFormal(const Pos & pos, Formals * formals, const Formal & formal)
static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols,
vector<std::pair<Pos, std::variant<Expr *, StringToken> > > & es)
{
if (es.empty()) return new ExprString(symbols.create(""));
if (es.empty()) return new ExprString("");

/* Figure out the minimum indentation. Note that by design
whitespace-only final lines are not taken into account. (So
Expand Down Expand Up @@ -244,7 +244,7 @@ static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols,
s2 = string(s2, 0, p + 1);
}

es2->emplace_back(i->first, new ExprString(symbols.create(s2)));
es2->emplace_back(i->first, new ExprString(s2));
};
for (; i != es.end(); ++i, --n) {
std::visit(overloaded { trimExpr, trimString }, i->second);
Expand Down Expand Up @@ -434,7 +434,7 @@ expr_simple
$$ = new ExprCall(CUR_POS,
new ExprVar(data->symbols.create("__findFile")),
{new ExprVar(data->symbols.create("__nixPath")),
new ExprString(data->symbols.create(path))});
new ExprString(path)});
}
| URI {
static bool noURLLiterals = settings.isExperimentalFeatureEnabled(Xp::NoUrlLiterals);
Expand All @@ -443,7 +443,7 @@ expr_simple
.msg = hintfmt("URL literals are disabled"),
.errPos = CUR_POS
});
$$ = new ExprString(data->symbols.create($1));
$$ = new ExprString(string($1));
}
| '(' expr ')' { $$ = $2; }
/* Let expressions `let {..., body = ...}' are just desugared
Expand All @@ -458,19 +458,19 @@ expr_simple
;

string_parts
: STR { $$ = new ExprString(data->symbols.create($1)); }
: STR { $$ = new ExprString(string($1)); }
| string_parts_interpolated { $$ = new ExprConcatStrings(CUR_POS, true, $1); }
| { $$ = new ExprString(data->symbols.create("")); }
| { $$ = new ExprString(""); }
;

string_parts_interpolated
: string_parts_interpolated STR
{ $$ = $1; $1->emplace_back(makeCurPos(@2, data), new ExprString(data->symbols.create($2))); }
{ $$ = $1; $1->emplace_back(makeCurPos(@2, data), new ExprString(string($2))); }
| string_parts_interpolated DOLLAR_CURLY expr '}' { $$ = $1; $1->emplace_back(makeCurPos(@2, data), $3); }
| DOLLAR_CURLY expr '}' { $$ = new vector<std::pair<Pos, Expr *> >; $$->emplace_back(makeCurPos(@1, data), $2); }
| STR DOLLAR_CURLY expr '}' {
$$ = new vector<std::pair<Pos, Expr *> >;
$$->emplace_back(makeCurPos(@1, data), new ExprString(data->symbols.create($1)));
$$->emplace_back(makeCurPos(@1, data), new ExprString(string($1)));
$$->emplace_back(makeCurPos(@2, data), $3);
}
;
Expand Down Expand Up @@ -524,7 +524,7 @@ attrs
{ $$ = $1;
ExprString * str = dynamic_cast<ExprString *>($2);
if (str) {
$$->push_back(AttrName(str->s));
$$->push_back(AttrName(data->symbols.create(str->s)));
delete str;
} else
throw ParseError({
Expand All @@ -541,7 +541,7 @@ attrpath
{ $$ = $1;
ExprString * str = dynamic_cast<ExprString *>($3);
if (str) {
$$->push_back(AttrName(str->s));
$$->push_back(AttrName(data->symbols.create(str->s)));
delete str;
} else
$$->push_back(AttrName($3));
Expand All @@ -551,7 +551,7 @@ attrpath
{ $$ = new vector<AttrName>;
ExprString *str = dynamic_cast<ExprString *>($1);
if (str) {
$$->push_back(AttrName(str->s));
$$->push_back(AttrName(data->symbols.create(str->s)));
delete str;
} else
$$->push_back(AttrName($1));
Expand Down

0 comments on commit 9ac836d

Please sign in to comment.