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

Avoid allocations when joining strings #7776

Merged
merged 2 commits into from Sep 29, 2015
Merged

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented Sep 28, 2015

Review on Reviewable

frewsxcv added 2 commits Sep 28, 2015
Instead of intermediate allocations of `Vec`s, we should utilize
`str_join` which operates on iterators
@frewsxcv frewsxcv changed the title Link to 'ask for a reset' tracking issue Avoid allocations when joining strings Sep 28, 2015
@mbrubeck
Copy link
Contributor

mbrubeck commented Sep 29, 2015

Reviewed 1 of 1 files at r1, 4 of 4 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@mbrubeck
Copy link
Contributor

mbrubeck commented Sep 29, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

📌 Commit f14f09e has been approved by mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

Testing commit f14f09e with merge 5b5114d...

bors-servo pushed a commit that referenced this pull request Sep 29, 2015
Avoid allocations when joining strings



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7776)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

💔 Test failed - mac-rel-wpt

@frewsxcv
Copy link
Member Author

frewsxcv commented Sep 29, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

Testing commit f14f09e with merge 4823ec9...

bors-servo pushed a commit that referenced this pull request Sep 29, 2015
Avoid allocations when joining strings



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7776)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 29, 2015

@bors-servo bors-servo merged commit f14f09e into servo:master Sep 29, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@frewsxcv frewsxcv deleted the frewsxcv:str-join branch Sep 29, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 30, 2015

This changes behaviour in the face of empty strings in the iterator, I think. Did you confirm that can't happen?

@frewsxcv
Copy link
Member Author

frewsxcv commented Sep 30, 2015

How does the behavior change with empty strings?

@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 30, 2015

Joining ["", "x"] would have a leading separator before, I think, and won't now.

@frewsxcv
Copy link
Member Author

frewsxcv commented Sep 30, 2015

@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 30, 2015

@frewsxcv
Copy link
Member Author

frewsxcv commented Sep 30, 2015

Ah, that's what you meant. I'm not sure about cssstyledeclaration.rs, but with the other two DOM instances, I think we're just going to eventually strip away the whitespace (they're being joined together by " ") so it shouldn't affect the generated value AFAIK

@mbrubeck
Copy link
Contributor

mbrubeck commented Sep 30, 2015

In cssstyledeclaration, this is run on the result of longhands_from_shorthand which returns a list of static strings that are never empty.

@frewsxcv
Copy link
Member Author

frewsxcv commented Sep 30, 2015

I'm planning to open a PR later that will make str_join functionally consistent with the builtin join method

frewsxcv added a commit to frewsxcv/servo that referenced this pull request Sep 30, 2015
Prior to this commit, `str_join` would skip empty items at the start of
the `Iterator` until it found a non-empty item. This contradicts
`SliceConcatExt::join` which doesn't skip anything.

Brought up in:

servo#7776 (comment)
@frewsxcv
Copy link
Member Author

frewsxcv commented Sep 30, 2015

PR opened #7806

bors-servo pushed a commit that referenced this pull request Oct 1, 2015
Make util::str::str_join consistent with SliceConcatExt::join

Prior to this commit, `str_join` would skip empty items at the start of
the `Iterator` until it found a non-empty item. This contradicts
`SliceConcatExt::join` which doesn't skip anything.

Brought up in:

#7776 (comment)

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7806)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this pull request Oct 1, 2015
Make util::str::str_join consistent with SliceConcatExt::join

Prior to this commit, `str_join` would skip empty items at the start of
the `Iterator` until it found a non-empty item. This contradicts
`SliceConcatExt::join` which doesn't skip anything.

Brought up in:

#7776 (comment)

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7806)
<!-- Reviewable:end -->
jrmuizel pushed a commit to jrmuizel/gecko-cinnabar that referenced this pull request Jun 12, 2017
…ncatExt::join (from frewsxcv:consistent-str-join); r=mbrubeck

Prior to this commit, `str_join` would skip empty items at the start of
the `Iterator` until it found a non-empty item. This contradicts
`SliceConcatExt::join` which doesn't skip anything.

Brought up in:

servo/servo#7776 (comment)

Source-Repo: https://github.com/servo/servo
Source-Revision: 520c907742afbb94085c6b5e62156a5457530010
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Sep 30, 2019
…ncatExt::join (from frewsxcv:consistent-str-join); r=mbrubeck

Prior to this commit, `str_join` would skip empty items at the start of
the `Iterator` until it found a non-empty item. This contradicts
`SliceConcatExt::join` which doesn't skip anything.

Brought up in:

servo/servo#7776 (comment)

Source-Repo: https://github.com/servo/servo
Source-Revision: 520c907742afbb94085c6b5e62156a5457530010

UltraBlame original commit: ce33507715eacf6a421095c1da08d2c4326d34ef
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…ncatExt::join (from frewsxcv:consistent-str-join); r=mbrubeck

Prior to this commit, `str_join` would skip empty items at the start of
the `Iterator` until it found a non-empty item. This contradicts
`SliceConcatExt::join` which doesn't skip anything.

Brought up in:

servo/servo#7776 (comment)

Source-Repo: https://github.com/servo/servo
Source-Revision: 520c907742afbb94085c6b5e62156a5457530010

UltraBlame original commit: ce33507715eacf6a421095c1da08d2c4326d34ef
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…ncatExt::join (from frewsxcv:consistent-str-join); r=mbrubeck

Prior to this commit, `str_join` would skip empty items at the start of
the `Iterator` until it found a non-empty item. This contradicts
`SliceConcatExt::join` which doesn't skip anything.

Brought up in:

servo/servo#7776 (comment)

Source-Repo: https://github.com/servo/servo
Source-Revision: 520c907742afbb94085c6b5e62156a5457530010

UltraBlame original commit: ce33507715eacf6a421095c1da08d2c4326d34ef
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.