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

Add ProgramCacheObserver for an application can handle ProgramCache update #2764

Merged
merged 1 commit into from May 25, 2018

Conversation

sotaroikeda
Copy link
Contributor

@sotaroikeda sotaroikeda commented May 17, 2018

The pull request has some changes that application can pre-load program binary to ProgramCache and can handle ProgramCache update for saving program binary.

Related info is in Bug 1418202


This change is Reviewable

@sotaroikeda
Copy link
Contributor Author

r? anyone.

pub struct ProgramBinary {
sources: ProgramSources,
Copy link
Member

Choose a reason for hiding this comment

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

can we only have this field for when serialize_program is featured?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I am going to address it.

binary,
format
}
}
}

/// The interfaces that an application can implement to handle ProgramCache update
pub trait ProgramCacheObserver {
fn notify_binary_added(&self, program_binary: &Arc<ProgramBinary>);
Copy link
Member

Choose a reason for hiding this comment

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

does the application need to have Arc<> here? would it make sense to provide &ProgramBinary instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arc<> is necessary here, since ProgramBinary is serialized and is stored to disk on worker thread.

@@ -48,7 +48,7 @@ extern crate cfg_if;
extern crate lazy_static;
#[macro_use]
extern crate log;
#[cfg(any(feature = "debugger", feature = "capture", feature = "replay"))]
#[cfg(any(feature = "debugger", feature = "capture", feature = "replay", feature = "serialize_program"))]
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can just do feature = "serde" here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update it in a next pull request.

}
)
}
pub fn load_program_binary(&self, program_binary: Arc<ProgramBinary>) {
Copy link
Member

Choose a reason for hiding this comment

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

where is this used or meant to be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Application uses the function to store ProgramBinary to ProgramCache. The ProgramBinary is loaded from disk.

@@ -15,6 +15,7 @@ capture = ["webrender_api/serialize", "ron", "serde", "debug_renderer"]
replay = ["webrender_api/deserialize", "ron", "serde"]
debug_renderer = []
pathfinder = ["pathfinder_font_renderer", "pathfinder_gfx_utils", "pathfinder_partitioner", "pathfinder_path_utils"]
serialize_program = ["serde"]
Copy link
Member

Choose a reason for hiding this comment

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

need to add testing for it to CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add the testing.

@sotaroikeda
Copy link
Contributor Author

I applied the comment, but Arc was kept since since ProgramBinary is serialized and is stored to disk on worker thread. diagram shows gecko side implementation.

@kvark can you review it again?

@kvark
Copy link
Member

kvark commented May 25, 2018

thank you!
@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit f478f0a has been approved by kvark

@bors-servo
Copy link
Contributor

⌛ Testing commit f478f0a with merge 264a26e...

bors-servo pushed a commit that referenced this pull request May 25, 2018
Add ProgramCacheObserver for an application can handle ProgramCache update

The pull request has some changes that  application can pre-load program binary to ProgramCache and can handle ProgramCache update for saving program binary.

Related info is in [Bug 1418202](https://bugzilla.mozilla.org/show_bug.cgi?id=1418202)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2764)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 264a26e to master...

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.

None yet

3 participants