[libSyntax] Add a reference counted version of OwnedString #18677
Conversation
|
@swift-ci Please smoke test |
ahoppen
force-pushed
the
ref-counted-owned-string
branch
from
6a34c28 to
f38e33e
Compare
|
After this change, where do we use the |
ahoppen
force-pushed
the
ref-counted-owned-string
branch
from
f38e33e to
f9e417c
Compare
|
@swift-ci Please smoke test |
| assert(substring && "expected successful malloc of copy"); | ||
| public: | ||
| static TextOwner *make(StringRef Text) { | ||
| auto size = totalSizeToAlloc<char>(Text.size()); |
There was a problem hiding this comment.
Should these be NUL-terminated?
There was a problem hiding this comment.
Since StringRef already contains a length, there is no need to NUL terminate them.
include/swift/Basic/OwnedString.h
Outdated
| /// \c OwnedString will not take ownership of that buffer and will assume that | ||
| /// the buffer outlives its lifetime. | ||
| static OwnedString makeUnowned(StringRef Str) { | ||
| return OwnedString(Str.data(), /*OwnedPtr=*/nullptr); |
There was a problem hiding this comment.
This is going to lose the length, which might be incorrect.
There was a problem hiding this comment.
Whoops. That carried over from the old implementation. Fixed now.
include/swift/Basic/OwnedString.h
Outdated
| return makeUnowned(Str); | ||
| } else { | ||
| llvm::IntrusiveRefCntPtr<TextOwner> OwnedPtr(TextOwner::make(Str)); | ||
| return OwnedString(StringRef(OwnedPtr->getText(), Str.size()), OwnedPtr); |
There was a problem hiding this comment.
Nitpick: using std::move here will avoid a retain/release.
We cannot use unowned strings for token texts of incrementally parsed syntax trees since the source buffer to which reused nodes refer will have been freed for reused nodes. Always copying the token text whenever OwnedString is passed is too expensive. A reference counted copy of the string allows us to keep the token's string alive across incremental parses while eliminating unnecessary copies.
ahoppen
force-pushed
the
ref-counted-owned-string
branch
from
f9e417c to
ac512d4
Compare
|
@swift-ci Please smoke test |
| presence, /*Arena=*/nullptr, nodeId); | ||
| value = swift::RawSyntax::make( | ||
| tokenKind, swift::OwnedString::makeRefCounted(text), leadingTrivia, | ||
| trailingTrivia, presence, /*Arena=*/nullptr, nodeId); |
There was a problem hiding this comment.
How about make RawSyntax::make() to receive StringRef, then construct OwnedString in it?
There was a problem hiding this comment.
Ah, you want to control Owned/Unowned in call site. OK then.
There was a problem hiding this comment.
Yeah, that was my idea behind it.
| @@ -454,7 +454,8 @@ struct MappingTraits<swift::syntax::TriviaPiece> { | |||
| % else: | |||
| StringRef text; | |||
| in.mapRequired("value", text); | |||
| return swift::syntax::TriviaPiece(kind, text); | |||
| return swift::syntax::TriviaPiece( | |||
| kind, swift::OwnedString::makeRefCounted(text)); | |||
There was a problem hiding this comment.
Ditto for TriviaPiece (receive StringRef).
|
|
||
| memcpy(substring, Data, Length); | ||
| substring[Length] = '\0'; | ||
| const char *getText() const { return getTrailingObjects<char>(); } |
There was a problem hiding this comment.
Is there any specific reasons to put char* as a trailing object instead of a regular member?
There was a problem hiding this comment.
It allows us to allocate the storage with a single allocation instead of two which should be less overhead.
|
LGTM now! |
We cannot use unowned strings for token texts of incrementally parsed syntax trees since the source buffer to which reused nodes refer will have been freed for reused nodes. Always copying the token text whenever
OwnedStringis passed is too expensive. A reference counted copy of the string allows us to keep the token's string alive across incremental parses while eliminating unnecessary copies.