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

Remove NodeId usage from ChromiumSenderThread #356

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

noituri
Copy link
Member

@noituri noituri commented Jan 19, 2024

Initially, when I implemented the web renderer I thought that it would be possible to have one web renderer that is used by multiple web views. In this PR I removed some of the unnecessary stuff that was added because of the assumptions I made when I was implementing it.

Issue: #352

@noituri noituri force-pushed the @noituri/chromium-thread-node-id branch from af669d9 to 50d2589 Compare January 23, 2024 16:02
@noituri noituri marked this pull request as ready for review January 23, 2024 16:03
Copy link
Collaborator

@wkozyra95 wkozyra95 left a comment

Choose a reason for hiding this comment

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

Please verify if this approach works (does not crash) even without validation that only one component is using the renderer.


pub fn resize(&mut self, size: usize) -> Result<(), SharedMemoryError> {
// Releasing the current `Shmem` instance to ensure it does not erase the shared memory descriptor from the file system
// This is critical to ensure when a new `Shmem` is created at the same location, it doesn't conflict with the old descriptor
drop(self.inner.take());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
drop(self.inner.take());
self.inner.take();

wouldn't this be the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

This whole line is not needed unless I forgot about something.

}
}

// Create additional shared memory
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if the number of inputs is smaller, there is no cleanup of already allocated memory

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no cleanup since it requires synchronization. It will be added eventually

frame.send_process_message(cef::ProcessId::Renderer, process_message)?;
// -----

shmem.resize(size)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if you only resize if shm is too small? Would this avoid potential crashes even without synchronization?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would work

@wkozyra95
Copy link
Collaborator

I need those changes for the other PR I'm working on and it seems everyone already reviewed it, so I'll merge it. Please take a look at the comments and if there is anything actionable there address that in a follow-up PR.

@wkozyra95 wkozyra95 merged commit 86853b3 into master Feb 2, 2024
1 check passed
@wkozyra95 wkozyra95 deleted the @noituri/chromium-thread-node-id branch February 2, 2024 13:33
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

4 participants