Skip to content

Conversation

@iDawer
Copy link
Contributor

@iDawer iDawer commented Apr 1, 2021

In rust-analyzer/rowan#101 rowan::SyntaxNode::green returns Cow<'_, GreenNodeData>. It returns borrow of green node of immutable syntax tree node.
Using this we can return borrowed text from ast::Name::text.

However now it allocates in case of mutable syntax trees. (see next comment)

The idea comes from rust-analyzer/rowan#100 (comment)

@iDawer
Copy link
Contributor Author

iDawer commented Apr 1, 2021

Replaced Cow<str> with cow-like TokenText:

pub enum TokenText<'a> {
    Borrowed(&'a str),
    Owned(GreenToken)
}

Though, it does not implement ToOwned.

The draft is waiting for:

@iDawer iDawer force-pushed the syntax-borrow-green-node branch from 47473f5 to eafef03 Compare April 13, 2021 09:03
@iDawer iDawer marked this pull request as ready for review April 13, 2021 10:22
@matklad
Copy link
Contributor

matklad commented Apr 13, 2021

would be interesting to get some benchmakrs here. You can run cargo xtask bb baseline / cargo xtask bb cow to prepare two test binaries, and /target/rust-analyzer-last-cow -q analysis-stats --memory-usage . to run a simple bench.

@iDawer
Copy link
Contributor Author

iDawer commented Apr 13, 2021

With cpupower frequency-set -g performance applied:
master 27e80e9 vs this f316163

[dawer@iPC rust-analyzer]$ ./target/rust-analyzer-baseline_27e80e9 -q analysis-stats --memory-usage .
Database loaded:     946.71ms, 283minstr, 86mb
  crates: 34, mods: 649, decls: 12591, fns: 9924
Item Collection:     19.01s, 77ginstr, 288mb
  exprs: 278045, ??ty: 2512 (0%), ?ty: 1870 (0%), !ty: 1064
Inference:           46.72s, 120ginstr, 441mb
Total:               65.73s, 197ginstr, 729mb

[dawer@iPC rust-analyzer]$ ./target/rust-analyzer-cow_f316163 -q analysis-stats --memory-usage .
Database loaded:     949.80ms, 283minstr, 86mb
  crates: 34, mods: 649, decls: 12591, fns: 9924
Item Collection:     19.04s, 77ginstr, 288mb
  exprs: 278045, ??ty: 2512 (0%), ?ty: 1870 (0%), !ty: 1064
Inference:           46.91s, 120ginstr, 441mb
Total:               65.95s, 197ginstr, 729mb

@matklad
Copy link
Contributor

matklad commented May 3, 2021

LGTM, @iDawer can you rebase?

bors d+

@bors
Copy link
Contributor

bors bot commented May 3, 2021

✌️ iDawer can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@iDawer
Copy link
Contributor Author

iDawer commented May 3, 2021

Hmm, version 0.13.0-pre.5 introduced breaking change in "pre" version, is it ok?
Currently, rowan dependency is pinned on "=0.13.0-pre.3" (#8551) because of this.
I'm uncertain, shouldn't we bump rowan's minor version with the change + revert the change in 0.13.0-pre.6 to be in line with semantic versioning?

@matklad
Copy link
Contributor

matklad commented May 3, 2021

Breaking changes in pre version are OK -- that's exactly what makes pre versions useful!

@iDawer iDawer force-pushed the syntax-borrow-green-node branch from f316163 to 6ad368e Compare May 3, 2021 18:32
@iDawer
Copy link
Contributor Author

iDawer commented May 3, 2021

I left rowan version pinned rowan = "=0.13.0-pre.5"

bors r+

@iDawer iDawer force-pushed the syntax-borrow-green-node branch from 935a1bb to 90a5dca Compare May 6, 2021 05:30
@iDawer
Copy link
Contributor Author

iDawer commented May 6, 2021

bors r+

@Veykril
Copy link
Member

Veykril commented May 6, 2021

bors retry

@bors
Copy link
Contributor

bors bot commented May 6, 2021

@bors bors bot merged commit 0ee945e into rust-lang:master May 6, 2021
@Veykril Veykril changed the title Borrow text of immutable syntax node internal: Borrow text of immutable syntax node May 8, 2021
@iDawer iDawer deleted the syntax-borrow-green-node branch May 19, 2021 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants