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.

Sign up for GitHub

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

Jump to bottom

Reland "Bug 1842221 - Simplify ThinArc and friends" #28

Merged
merged 3 commits into from
Apr 9, 2024
Merged

Reland "Bug 1842221 - Simplify ThinArc and friends" #28

merged 3 commits into from
Apr 9, 2024

Conversation

Loirooriol
Copy link
Contributor

@Loirooriol Loirooriol commented Apr 3, 2024 •
edited
Loading

I reverted 516aec4 because it was making Servo crash.
This relands it together with 63a6a2d, with avoids the crashes.

Loirooriol added a commit to Loirooriol/servo that referenced this pull request Apr 3, 2024
@Loirooriol
Bump Stylo to servo/stylo#28
8a95265
@Loirooriol
Copy link
Contributor Author

Still some crashes, like for

<!DOCTYPE html>
<div id="el">CRASH?</div>
<script>
for (let i = 0; i < 1000; i++) {
  el.matches("div[id=el]");
}
</script>
0x000055a643a6dc9c in edata_arena_ind_get (edata=0x0) at include/jemalloc/internal/edata.h:258
258             unsigned arena_ind = (unsigned)((edata->e_bits &
(rr) backtrace
#0  0x000055a643a6dc9c in edata_arena_ind_get (edata=0x0) at include/jemalloc/internal/edata.h:258
#1  0x000055a643a65534 in tcache_bin_flush_match (edata=0x0, cur_arena_ind=0, cur_binshard=0, small=true) at src/tcache.c:301
#2  tcache_bin_flush_impl (tsd=0x7fae34e7c198, tcache=0x7fae34e7c4f0, cache_bin=0x7fae34e7c558, binind=4, ptrs=0x7fae34e67758, nflush=64, small=true) at src/tcache.c:434
#3  tcache_bin_flush_bottom (tsd=0x7fae34e7c198, tcache=0x7fae34e7c4f0, cache_bin=0x7fae34e7c558, binind=4, rem=64, small=true) at src/tcache.c:519
#4  _rjem_je_tcache_bin_flush_small (tsd=0x7fae34e7c198, tcache=0x7fae34e7c4f0, cache_bin=0x7fae34e7c558, binind=4, rem=64) at src/tcache.c:529
#5  0x000055a6439b1239 in tcache_dalloc_small (tsd=0x7fae34e7c198, tcache=0x7fae34e7c4f0, ptr=0x7fae35d16800, binind=4, slow_path=false) at include/jemalloc/internal/tcache_inlines.h:157
#6  arena_sdalloc (tsdn=0x7fae34e7c198, ptr=0x7fae35d16800, size=64, tcache=0x7fae34e7c4f0, caller_alloc_ctx=0x7fae34e67bc0, slow_path=false) at include/jemalloc/internal/arena_inlines_b.h:418
#7  isdalloct (tsdn=0x7fae34e7c198, ptr=0x7fae35d16800, size=64, tcache=0x7fae34e7c4f0, alloc_ctx=0x7fae34e67bc0, slow_path=false) at include/jemalloc/internal/jemalloc_internal_inlines_c.h:133
#8  isfree (tsd=0x7fae34e7c198, ptr=0x7fae35d16800, usize=64, tcache=0x7fae34e7c4f0, slow_path=false) at src/jemalloc.c:2982
#9  _rjem_je_sdallocx_default (ptr=0x7fae35d16800, size=56, flags=0) at src/jemalloc.c:3924
#10 0x000055a6439b46be in _rjem_sdallocx (ptr=0x7fae35d16800, size=56, flags=0) at src/jemalloc.c:3939
#11 0x000055a643986804 in jemallocator::{impl#0}::dealloc (self=0x55a647fa1e0f, ptr=0x7fae35d16800, layout=...) at /home/oriol/.cargo/registry/src/index.crates.io-6f17d22bba15001f/jemallocator-0.5.4/src/lib.rs:129
#12 0x000055a643986433 in servo_allocator::_::__rust_dealloc (ptr=0x7fae35d16800, size=56, align=8) at components/allocator/lib.rs:8
#13 0x000055a645abe3fb in alloc::alloc::dealloc () at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/alloc/src/alloc.rs:117
#14 alloc::alloc::{impl#1}::deallocate (self=0x7fae34e698b8, ptr=..., layout=...) at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/alloc/src/alloc.rs:254
#15 0x000055a645a7b218 in alloc::boxed::{impl#8}::drop<servo_arc::ArcInner<servo_arc::HeaderSlice<selectors::builder::SpecificityAndFlags, selectors::parser::Component<style::servo::selector_parser::SelectorImpl>>>, alloc::alloc::Global> (self=0x7fae34e698b0) at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/alloc/src/boxed.rs:1235
#16 0x000055a645a5b847 in core::ptr::drop_in_place<alloc::boxed::Box<servo_arc::ArcInner<servo_arc::HeaderSlice<selectors::builder::SpecificityAndFlags, selectors::parser::Component<style::servo::selector_parser::SelectorImpl>>>, alloc::alloc::Global>> () at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/core/src/ptr/mod.rs:497
#17 0x000055a645cc065a in servo_arc::Arc<servo_arc::HeaderSlice<selectors::builder::SpecificityAndFlags, selectors::parser::Component<style::servo::selector_parser::SelectorImpl>>>::drop_slow<servo_arc::HeaderSlice<selectors::builder::SpecificityAndFlags, selectors::parser::Component<style::servo::selector_parser::SelectorImpl>>> (self=0x7fae34e69a78) at /home/oriol/src/stylo/servo_arc/lib.rs:341
#18 0x000055a645a74119 in servo_arc::{impl#14}::drop<servo_arc::HeaderSlice<selectors::builder::SpecificityAndFlags, selectors::parser::Component<style::servo::selector_parser::SelectorImpl>>> (self=0x7fae34e69a78)
    at /home/oriol/src/stylo/servo_arc/lib.rs:532
#19 0x000055a645a5939b in core::ptr::drop_in_place<servo_arc::Arc<servo_arc::HeaderSlice<selectors::builder::SpecificityAndFlags, selectors::parser::Component<style::servo::selector_parser::SelectorImpl>>>> ()
    at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/core/src/ptr/mod.rs:497
#20 0x000055a645a7208b in core::ptr::drop_in_place<selectors::parser::Selector<style::servo::selector_parser::SelectorImpl>> () at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/core/src/ptr/mod.rs:497
#21 0x000055a645a4b51a in core::ptr::drop_in_place<[selectors::parser::Selector<style::servo::selector_parser::SelectorImpl>]> () at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/core/src/ptr/mod.rs:497
#22 0x000055a645a76386 in smallvec::{impl#38}::drop<[selectors::parser::Selector<style::servo::selector_parser::SelectorImpl>; 1]> (self=0x7fae34e69a78)
    at /home/oriol/.cargo/registry/src/index.crates.io-6f17d22bba15001f/smallvec-1.13.2/src/lib.rs:2124
#23 0x000055a645a52b3b in core::ptr::drop_in_place<smallvec::SmallVec<[selectors::parser::Selector<style::servo::selector_parser::SelectorImpl>; 1]>> () at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/core/src/ptr/mod.rs:497
#24 0x000055a645a49eeb in core::ptr::drop_in_place<selectors::parser::SelectorList<style::servo::selector_parser::SelectorImpl>> () at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/core/src/ptr/mod.rs:497
#25 0x000055a640427276 in script::dom::element::{impl#8}::Matches (self=0x7fae355f2300, selectors=...) at components/script/dom/element.rs:2774

@Loirooriol Loirooriol force-pushed the reland-simply-thinarc-and-friends branch from 54911f3 to 80578c0 Compare April 4, 2024 15:38
Loirooriol added a commit to Loirooriol/servo that referenced this pull request Apr 4, 2024
@Loirooriol
Bump Stylo to servo/stylo#28
8dd9a12
@zrhoffman
Copy link
Contributor

Maybe it's worth adding a crashtest in #28? Behavior is unchanged upstream, AFAICT.

@Loirooriol
Copy link
Contributor Author

#28 (comment) is just a reduction of an existing WPT test, so adding it as a crashtest isn't probably worth it.

zrhoffman
zrhoffman approved these changes Apr 7, 2024
View reviewed changes
Copy link
Contributor

@zrhoffman zrhoffman left a comment

Choose a reason for hiding this comment

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

Looks good once #28 is updated to include the changes in <https://phabricator.services.mozilla.com/D206659?vs=845283&id=845659>.

@emilio @Loirooriol
Bug 1842221 - Simplify ThinArc and friends. r=boris
5e10d0f 
ThinArc is way more complex than it needs to be, and miri complains
about various things when using the selectors crate with it.

This doesn't fully fix miri, but it gets much closer. ThinArc becomes
just an alias for Arc<HeaderSlice<>>. This allows to simplify the
bindings to ArcSlice too, since now the existing Arc<> code can just be
used.

Differential Revision: https://phabricator.services.mozilla.com/D183011
@Loirooriol Loirooriol force-pushed the reland-simply-thinarc-and-friends branch from 80578c0 to 3b1b786 Compare April 8, 2024 10:24
@Loirooriol Loirooriol requested a review from mrobinson April 8, 2024 11:57
@Loirooriol
Copy link
Contributor Author

Done. But @zrhoffman I guess you don't have write access here since it still says "review required".

@Loirooriol Loirooriol force-pushed the reland-simply-thinarc-and-friends branch from 3b1b786 to 263c660 Compare April 8, 2024 14:14
mrobinson
mrobinson approved these changes Apr 9, 2024
View reviewed changes
Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

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

This review is a rubber stamp, because this is a port of a Gecko change and also it has already been reviewed by @zrhoffman.

@Loirooriol Loirooriol added this pull request to the merge queue Apr 9, 2024
Merged via the queue into servo:main with commit 644ff83 Apr 9, 2024
3 checks passed
@Loirooriol Loirooriol deleted the reland-simply-thinarc-and-friends branch April 9, 2024 12:26
Loirooriol added a commit to Loirooriol/servo that referenced this pull request Apr 9, 2024
@Loirooriol
Bump Stylo to servo/stylo#28
a02fc2b
github-merge-queue bot pushed a commit to servo/servo that referenced this pull request Apr 9, 2024
@Loirooriol
Bump Stylo to servo/stylo#28 (#31991)
7ef1baf
github-merge-queue bot pushed a commit to servo/servo that referenced this pull request Apr 9, 2024
@Loirooriol
Bump Stylo to servo/stylo#28 (#31991)
586a708
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zrhoffman zrhoffman zrhoffman approved these changes

@mrobinson mrobinson mrobinson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Loirooriol @zrhoffman @mrobinson @emilio