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

Purge pipelines from distant history. #11893

Closed
wants to merge 1 commit into from
Closed

Conversation

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 28, 2016

This change is Reviewable

@Ms2ger

This comment has been minimized.

Copy link
Contributor Author

Ms2ger commented Jun 28, 2016

@asajeffrey

This comment has been minimized.

Copy link
Member

asajeffrey commented Jun 28, 2016

cc @ConnorGBrewster

@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Jun 28, 2016

Can someone summarise what that does?

Is history still conserved even if pipelines are purged?
Are non-accessible pipelines (pipeline1 navigate to pipeline2, goBack to pipeline1, navigate to pipeline3, pipeline2 is not accessible anymore) discarded (maybe it was already)?

@cbrewster

This comment has been minimized.

Copy link
Member

cbrewster commented Jun 28, 2016

@paulrouget I believe the idea is that we will keep 3(number can change) entries in the history, any pipelines past that will get closed. Once those pipelines are traversed to again, they are reloaded. This keeps us from having many many pipelines open after navigating a lot.

@paulrouget

This comment has been minimized.

Copy link
Contributor

paulrouget commented Jun 28, 2016

Once those pipelines are traversed to again, they are reloaded

Ok, so we keep the history even if the pipeline is purged. That's all I was worried about :)

@cbrewster

This comment has been minimized.

@asajeffrey

This comment has been minimized.

Copy link
Member

asajeffrey commented Jun 28, 2016

I'm quite concerned about what happens when pipelines share a script thread. If page A contains a same-origin iframe, which starts out at B1, then navigates to B2, B3 and B4. This will trigger Pipeline::exit on B1, which shares a script thread with A.

@asajeffrey

This comment has been minimized.

Copy link
Member

asajeffrey commented Jun 28, 2016

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.


components/constellation/constellation.rs, line 222 [r1] (raw file):

        id: PipelineId,
    },
    Dead {

The terminology of alive vs dead doesn't seem aligned with the spec. https://html.spec.whatwg.org/multipage/browsers.html#session-history has a note saying "when a Document is not active, it's possible for it to be discarded to free resources," defined in https://html.spec.whatwg.org/multipage/browsers.html#discard-a-document.


components/constellation/constellation.rs, line 224 [r1] (raw file):

    Dead {
        url: Url,
        is_private: bool,

Should the data saved here be LoadData?

Is the privacy associated with the history entry or with the frame? (That is, can we have a frame, some of whose entries are private and some are public?)


components/constellation/constellation.rs, line 244 [r1] (raw file):

    }

    fn load(&mut self,

Can we add a comment saying what the semantics of this function is?


components/constellation/constellation.rs, line 246 [r1] (raw file):

    fn load(&mut self,
            pipeline_id: PipelineId,
            pipelines: &HashMap<PipelineId, Pipeline>,

Passing the pipelines hash map around seems like a break in the abstraction barrier between Frame and Constellation.


components/constellation/constellation.rs, line 247 [r1] (raw file):

            pipeline_id: PipelineId,
            pipelines: &HashMap<PipelineId, Pipeline>,
            through_history: bool)

What is this parameter for?


components/constellation/constellation.rs, line 264 [r1] (raw file):

    }

    fn maybe_purge(&mut self, pipelines: &HashMap<PipelineId, Pipeline>) {

Shouldn't this functionality be in Constellation rather than Frame?


components/constellation/constellation.rs, line 273 [r1] (raw file):

                            None => continue,
                        };
                        pipeline.exit();

Doesn't this exit a pipeline even if the script thread is being used by its parent?


components/constellation/constellation.rs, line 1423 [r1] (raw file):

                    self.handle_load_start_msg(&pipeline_id);
                    self.push_pending_frame(pipeline_id, Some(prev), true);
                    self.compositor_proxy.send(ToCompositorMsg::ChangePageUrl(pipeline_id, url));

Should we set the window URL, even if it's a child iframe that caused this navigation?


Comments from Reviewable

@asajeffrey

This comment has been minimized.

Copy link
Member

asajeffrey commented Jun 28, 2016

As a general comment, it strikes me that this is one way of handling document discarding, but not the only way. Rather than having a per-frame hard limit on history, we could have a per-constellation soft limit.

@asajeffrey

This comment has been minimized.

Copy link
Member

asajeffrey commented Jul 18, 2016

@Ms2ger is this still active?

@Ms2ger

This comment has been minimized.

Copy link
Contributor Author

Ms2ger commented Jul 19, 2016

Yes, I'm going to get back to your review comments, hopefully this week.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jul 22, 2016

☔️ The latest upstream changes (presumably #11866) made this pull request unmergeable. Please resolve the merge conflicts.

@asajeffrey

This comment has been minimized.

Copy link
Member

asajeffrey commented Jul 29, 2016

@Ms2ger ping?

@Ms2ger

This comment has been minimized.

Copy link
Contributor Author

Ms2ger commented Sep 9, 2016

Rebased.

@jdm jdm removed the S-needs-rebase label Sep 9, 2016
let mut future = vec!();
for frame in self.full_frame_tree_iter(frame_id_root) {
future.extend(frame.next.iter().map(|entry| (entry.instant, entry.frame_id, entry.pipeline_id)));
future.extend(frame.next.iter().map(|entry| (entry.instant, entry.frame_id, entry.entry.clone())));

This comment has been minimized.

Copy link
@cbrewster

cbrewster Sep 9, 2016

Member

nit: maybe we should mess with the naming here to avoid entry.entry.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 10, 2016

☔️ The latest upstream changes (presumably #13211) made this pull request unmergeable. Please resolve the merge conflicts.

@cbrewster

This comment has been minimized.

Copy link
Member

cbrewster commented Sep 10, 2016

Review status: 0 of 1 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.


components/constellation/constellation.rs, line 305 [r3] (raw file):

    }

    fn maybe_purge(&mut self, pipelines: &HashMap<PipelineId, Pipeline>) {

Should we be purging everything but the previous/next x entries at the joint session history(top-level browsing context) level or at the individual session history/frame(browsing context) level?


Comments from Reviewable

@Ms2ger Ms2ger force-pushed the Ms2ger:purge-pipelines branch from 0e5023b to 583e8e3 Sep 12, 2016
@Ms2ger

This comment has been minimized.

Copy link
Contributor Author

Ms2ger commented Sep 13, 2016

Added some docs for load(); the rest of the comments seem like questions the two of you are more qualified to answer than I am.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 14, 2016

☔️ The latest upstream changes (presumably #11698) made this pull request unmergeable. Please resolve the merge conflicts.

@Ms2ger Ms2ger force-pushed the Ms2ger:purge-pipelines branch from 583e8e3 to 18ca07d Sep 14, 2016
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Sep 19, 2016

☔️ The latest upstream changes (presumably #13227) made this pull request unmergeable. Please resolve the merge conflicts.

Ms2ger added a commit that referenced this pull request Sep 20, 2016
The recently added replace argument makes it less readable, especially with
the second boolean argument I am adding in #11893.
bors-servo added a commit that referenced this pull request Sep 20, 2016
Inline push_pending_frame into its callers.

The recently added replace argument makes it less readable, especially with
the second boolean argument I am adding in #11893.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13334)
<!-- Reviewable:end -->
@Ms2ger Ms2ger force-pushed the Ms2ger:purge-pipelines branch from 18ca07d to 4d3703f Sep 20, 2016
@Ms2ger Ms2ger force-pushed the Ms2ger:purge-pipelines branch from 4d3703f to 556d837 Sep 21, 2016
@Ms2ger

This comment has been minimized.

Copy link
Contributor Author

Ms2ger commented Sep 21, 2016

I have no idea what this is doing anymore. @asajeffrey @ConnorGBrewster, wanna take over?

@asajeffrey

This comment has been minimized.

Copy link
Member

asajeffrey commented Sep 21, 2016

This is quite tricky, especially with sharing script threads (and there are bugs around when script threads are shared, sigh). We may need to postpone this until we get the sharing script threads story straight. Either that or we need to work out how to get a script thread to cope with some of its globals being dead, and some being alive.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Oct 7, 2016

☔️ The latest upstream changes (presumably #13498) made this pull request unmergeable. Please resolve the merge conflicts.

@metajack

This comment has been minimized.

Copy link
Contributor

metajack commented Oct 11, 2016

@asajeffrey is going to take over this work. He's planning to rework script thread sharing which will further impact this, and at this point it probably makes sense for him to just include purging in his plans.

@asajeffrey

This comment has been minimized.

Copy link
Member

asajeffrey commented Oct 11, 2016

@ConnorGBrewster: document unloading will be in our future 😄

@asajeffrey

This comment has been minimized.

Copy link
Member

asajeffrey commented Nov 17, 2016

#14211 fixes #633, so now I'm turning my attention to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.