Skip to content

core: Remove time limit from ExecutionLimit #15550

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 9 additions & 44 deletions core/src/limits.rs
Original file line number Diff line number Diff line change
@@ -1,64 +1,39 @@
use crate::context::UpdateContext;
use std::time::Duration;

/// Indication of how long execution is allowed to take.
///
/// Execution is limited by two mechanisms:
///
/// 1. An *operation limit*, or oplimit, which is a certain number of actions
/// executed, bytes processed, or other things that can be done before
/// checking...
/// 2. A *time limit*, which is checked every time the oplimit runs out.
/// If it has not yet expired, then the oplimit is refreshed and we
/// continue.
///
/// This two-tiered system is intended to reduce the overhead of enforcing
/// execution limits.
/// Execution is limited by *operation limit*, or oplimit,
/// which is a certain number of actions
/// executed, bytes processed, or other things that can be done.
///
/// What constitutes an "operation" is up to the user of this structure. It may
/// cover individual interpreter opcodes, bytes decoded in a data stream, or so
/// on.
pub struct ExecutionLimit {
/// How many operations remain before the next timelimit check.
/// How many operations remain before the limit is reached.
///
/// If `None`, then the execution limit is not enforced.
/// If `None`, then an execution limit is not enforced.
current_oplimit: Option<usize>,

/// The number of operations allowed between timelimit checks.
///
/// If `None`, then the execution limit is not enforced.
max_ops_per_check: Option<usize>,

/// The amount of time allowed to be taken regardless of the opcount.
time_limit: Duration,
}

impl ExecutionLimit {
/// Construct an execution limit that allows unlimited execution.
pub fn none() -> ExecutionLimit {
Self {
current_oplimit: None,
max_ops_per_check: None,
time_limit: Duration::MAX,
}
}

/// Construct an execution limit that checks the current wall-clock time
/// after a certain number of operations are executed.
pub fn with_max_ops_and_time(ops: usize, time_limit: Duration) -> ExecutionLimit {
pub fn with_max_ops(ops: usize) -> ExecutionLimit {
Self {
current_oplimit: Some(ops),
max_ops_per_check: Some(ops),
time_limit,
}
}

/// Create an execution limit that is already exhausted.
pub fn exhausted() -> ExecutionLimit {
Self {
current_oplimit: Some(0),
max_ops_per_check: Some(0),
time_limit: Duration::from_secs(0),
}
}

Expand All @@ -67,25 +42,15 @@ impl ExecutionLimit {
///
/// This is intended to be called after the given operations have been
/// executed. The ops will be deducted from the operation limit and, if
/// that limit is zero, the time limit will be checked. If both limits have
/// been breached, this returns `true`. Otherwise, this returns `false`,
/// and if the operation limit was exhausted, it will be returned to the
/// starting maximum.
/// that limit has been breached, this returns 'true'
pub fn did_ops_breach_limit(
&mut self,
context: &mut UpdateContext<'_, '_>,
_context: &mut UpdateContext<'_, '_>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the context parameter can be removed from here now?

Copy link
Member

Choose a reason for hiding this comment

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

Note that there's been quite some discussion about the point or validity of this PR on Discord.

ops: usize,
) -> bool {
if let Some(oplimit) = &mut self.current_oplimit {
*oplimit = oplimit.saturating_sub(ops);

if *oplimit == 0 {
if context.update_start.elapsed() >= self.time_limit {
return true;
}

self.current_oplimit = self.max_ops_per_check;
}
return *oplimit == 0;
}

false
Expand Down
2 changes: 1 addition & 1 deletion core/src/loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1929,7 +1929,7 @@ impl<'gc> Loader<'gc> {
Loader::preload_tick(
handle,
uc,
&mut ExecutionLimit::with_max_ops_and_time(10000, Duration::from_millis(1)),
&mut ExecutionLimit::with_max_ops(10000),
status,
redirected,
)?;
Expand Down
11 changes: 2 additions & 9 deletions core/src/player.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1590,16 +1590,9 @@ impl Player {

#[instrument(level = "debug", skip_all)]
pub fn run_frame(&mut self) {
let frame_time = Duration::from_nanos((750_000_000.0 / self.frame_rate) as u64);
let (mut execution_limit, may_execute_while_streaming) = match self.load_behavior {
LoadBehavior::Streaming => (
ExecutionLimit::with_max_ops_and_time(10000, frame_time),
true,
),
LoadBehavior::Delayed => (
ExecutionLimit::with_max_ops_and_time(10000, frame_time),
false,
),
LoadBehavior::Streaming => (ExecutionLimit::with_max_ops(10000), true),
LoadBehavior::Delayed => (ExecutionLimit::with_max_ops(10000), false),
LoadBehavior::Blocking => (ExecutionLimit::none(), false),
};
let preload_finished = self.preload(&mut execution_limit);
Expand Down