ronantakizawa/codereview-bench huggingface dataset:
Repository: parcel-bundler/parcel
File: packages/core/graph/src/Graph.js (JavaScript)

Code before:
        visited.add(nodeId);

        this.adjacencyList.forEachNodeIdConnectedFromReverse(nodeId, child => {
          if (!visited.has(child)) {
            queue.push(child);
          }
          return false;
        });
      } else {
        queue.pop();
        visit(nodeId, null, actions);

        if (stopped) {
          this._visited = visited;
          return;
        }
      }
    }

    this._visited = visited;
    return;
  }

  /**
   * Iterative implementation of DFS that supports all use-cases.
   *
   * This replaces `dfs` and will replace `dfsFast`.
   */
  dfs<TContext>({
    visit,
    startNodeId,
    getChildren,
  }: DFSParams<TContext>): ?TContext {
    let traversalStartNode = nullthrows(
      startNodeId ?? this.rootNodeId,
      'A start node is required to traverse',
    );
    this._assertHasNodeId(traversalStartNode);

    let visited;
    if (!this._visited || this._visited.capacity < this.nodes.length) {
      this._visited = new BitSet(this.nodes.length);
      visited = this._visited;
    } else {
      visited = this._visited;
      visited.clear();
    }
    // Take shared instance to avoid re-entrancy issues.
    this._visited = null;

    let stopped = false;

Review comment (performance):
What will be the approach for replacing `dfsFast`? Is it just changing the call sites? Will that require a separate flag / testing to ensure functionality is equivalent and performance doesn't regress?

Code after:

        this.adjacencyList.forEachNodeIdConnectedFromReverse(nodeId, child => {
          if (!visited.has(child)) {
            queue.push(child);
          }
          return false;
        });
      } else {
        queue.pop();
        visit(nodeId, null, actions);

        if (stopped) {
          this._visited = visited;
          return;
        }
      }
    }

    this._visited = visited;
    return;
  }

  /**
   * Iterative implementation of DFS that supports all use-cases.
   *
   * This replaces `dfs` and will replace `dfsFast`.
   */
  dfs<TContext>({
    visit,
    startNodeId,
    getChildren,
  }: DFSParams<TContext>): ?TContext {
    let traversalStartNode = nullthrows(
      startNodeId ?? this.rootNodeId,
      'A start node is required to traverse',
    );
    this._assertHasNodeId(traversalStartNode);

    let visited;
    if (!this._visited || this._visited.capacity < this.nodes.length) {
      this._visited = new BitSet(this.nodes.length);
      visited = this._visited;
    } else {
      visited = this._visited;
      visited.clear();
    }
    // Take shared instance to avoid re-entrancy issues.
    this._visited = null;

    let stopped = false;
    let skipped = false;
Repository: parcel-bundler/parcel
File: crates/node-bindings/src/parcel/parcel.rs (Rust)

Code before:
    _options: JsObject,
    callback: JsFunction,
  ) -> napi::Result<JsUndefined> {
    let (tx, rx) = channel();
    // Parse build options from JS options
    let build_options = BuildOptions {};

    // Call build in its own dedicated system thread
    thread::spawn({
      let parcel = self.parcel.clone();
      move || tx.send(parcel.build(build_options))
    });

    // JavaScript bindings to handle the build result
    let callback: ThreadsafeFunction<anyhow::Result<BuildResult>> = env
      .create_threadsafe_function(
        &callback,
        0,
        |ctx: ThreadSafeCallContext<anyhow::Result<BuildResult>>| match ctx.value {
          Ok(_) => Ok(vec![ctx.env.create_object()]),
          Err(err) => Err(napi::Error::from_reason(format!("{:?}", err))),
        },
      )?;

    // Asynchronously wait for a response from the build
    env.spawn_future(async move {
      if !matches!(
        callback.call(
          Ok(rx.await.unwrap()),
          ThreadsafeFunctionCallMode::NonBlocking
        ),
        Status::Ok
      ) {
        Err(napi::Error::from_reason("Unable to call callback"))
      } else {
        Ok(())
      }
    })?;

    env.get_undefined()
  }

  #[napi]
  pub fn default_thread_count(env: Env) -> napi::Result<JsNumber> {
    let cpus = num_cpus::get();
    let cpus = env.create_int32(cpus as i32)?;
    Ok(cpus)
  }

  // Temporary, for testing
  #[napi]

Review comment (suggestion):
I don't think you need tokio or thread safe function callbacks for this. You can use `env.create_deferred` to create a promise, and return that.

https://github.com/parcel-bundler/parcel/blob/c03a29ede215967a9ec4e472a1161eed0a913af6/crates/node-bindings/src/core.rs#L128-L137

Code after:
    let build_options = BuildOptions {};

    // Call build in its own dedicated system thread
    thread::spawn({
      let parcel = self.parcel.clone();
      move || match parcel.build(build_options) {
        Ok(_result) => deferred.resolve(|_env| Ok(ParcelNapiBuildResult {})),
        Err(error) => deferred.reject(napi::Error::from_reason(format!("{:?}", error))),
      }
    });

    Ok(promise)
  }

  // Temporary, for testing
  #[napi]
  pub async fn _testing_temp_fs_read_to_string(&self, path: String) -> napi::Result<String> {
    Ok(self.parcel.fs.read_to_string(&PathBuf::from(path))?)
  }

  #[napi]
  pub async fn _testing_temp_fs_is_file(&self, path: String) -> napi::Result<bool> {
    Ok(self.parcel.fs.is_file(&PathBuf::from(path)))
  }

  #[napi]
  pub async fn _testing_temp_fs_is_dir(&self, path: String) -> napi::Result<bool> {
    Ok(self.parcel.fs.is_dir(&PathBuf::from(path)))
  }

  #[napi]
  pub async fn _testing_rpc_ping(&self) -> napi::Result<()> {
    if self.parcel.rpc.as_ref().unwrap().ping().is_err() {
      return Err(napi::Error::from_reason("Failed to run"));
    }
    Ok(())
  }
}
Repository: parcel-bundler/parcel
File: crates/node-bindings/src/parcel/worker.rs (Rust)

Code before:
use std::sync::mpsc::Sender;

use napi::{Env, JsObject, JsUndefined};
use napi_derive::napi;
use parcel::rpc::nodejs::NodejsWorker;
use parcel_napi_helpers::JsTransferable;

#[napi]
pub fn register_worker(
  env: Env,
  channel: JsTransferable<Sender<NodejsWorker>>,
  worker: JsObject,
) -> napi::Result<JsUndefined> {
  let worker = NodejsWorker::new(worker)?;
  let tx_worker = channel.take()?;
  if tx_worker.send(worker).is_err() {
    return Err(napi::Error::from_reason("Unable to register worker"));
  }
  env.get_undefined()
}


Review comment (suggestion):
~~I don't think it's safe to send this across threads - https://github.com/parcel-bundler/parcel/blob/445b9bc0ca57922a20977ee81b50e7a07e35b6ab/crates/parcel_plugin_rpc/src/nodejs/rpc_conn_nodejs.rs#L11~~

Code after:
/// A Rust channel is transferred to the worker via JavaScript `worker.postMessage`.
/// The worker then calls `register_worker`, supplying it with an object containing
/// callbacks.
///
/// The callbacks are later called from the main thread to send work to the worker.
///
/// |-------------| --- Init channel ----> |-------------------|
/// | Main Thread |                        | Worker Thread (n) |
/// |-------------| <-- Worker wrapper --- |-------------------|
///
///                 **Later During Build**
///
///                 -- Resolver.resolve -->
///                 <- DependencyResult ---
///
///                 -- Transf.transform -->
///                 <--- Array<Asset> -----
#[napi]
pub fn register_worker(
  env: Env,
  channel: JsTransferable<Sender<NodejsWorker>>,
  worker: JsObject,
) -> napi::Result<JsUndefined> {
  let worker = NodejsWorker::new(worker)?;
  let tx_worker = channel.take()?;
  if tx_worker.send(worker).is_err() {
    return Err(napi::Error::from_reason("Unable to register worker"));
  }
  env.get_undefined()
}
Repository: parcel-bundler/parcel
File: packages/reporters/dev-server/src/HMRServer.js (JavaScript)

Code before:

  getSourceURL(asset: Asset): string {
    let origin = '';
    if (!this.options.devServer) {
      origin = `http://${this.options.host || 'localhost'}:${
        this.options.port
      }`;
    }
    return origin + HMR_ENDPOINT + '/' + asset.id;
  }

  handleSocketError(err: ServerError) {
    if (err.code === 'ECONNRESET') {
      // This gets triggered on page refresh, ignore this
      return;
    }

    this.options.logger.warn({
      origin: '@parcel/reporter-dev-server',
      message: `[${err.code}]: ${err.message}`,
      stack: err.stack,
    });
  }

  broadcast(msg: HMRMessage) {
    if (msg.type === 'update' && msg.assets.length > 10000) {
      // Split up message if too large
      for (let i = 0; i < msg.assets.length; i += 10000) {
        this.broadcast({
          ...msg,
          assets: msg.assets.slice(i, i + 10000),
        });
      }
    } else {
      const json = JSON.stringify(msg);
      for (let ws of this.wss.clients) {
        ws.send(json);
      }
    }
  }
}

function getSpecifier(dep: Dependency): string {
  if (typeof dep.meta.placeholder === 'string') {
    return dep.meta.placeholder;
  }

  return dep.specifier;
}


Review comment (refactor):
Is this just an arbitrary threshold that should be well under the limit?

Also, should this be extracted to a `const` rather than a magic number as it's referenced in a few places?

Code after:
        format: 'inline',
        sourceRoot: SOURCES_ENDPOINT + '/',
        // $FlowFixMe
        fs: asset.fs,
      });

      invariant(typeof sourcemapStringified === 'string');
      output += `\n//# sourceMappingURL=${sourcemapStringified}`;
      output += `\n//# sourceURL=${encodeURI(this.getSourceURL(asset))}\n`;
    }

    return output;
  }

  getSourceURL(asset: Asset): string {
    let origin = '';
    if (!this.options.devServer) {
      origin = `http://${this.options.host || 'localhost'}:${
        this.options.port
      }`;
    }
    return origin + HMR_ENDPOINT + '/' + asset.id;
  }

  handleSocketError(err: ServerError) {
    if (err.code === 'ECONNRESET') {
      // This gets triggered on page refresh, ignore this
      return;
    }

    this.options.logger.warn({
      origin: '@parcel/reporter-dev-server',
      message: `[${err.code}]: ${err.message}`,
      stack: err.stack,
    });
  }

  broadcast(msg: HMRMessage) {
    const json = JSON.stringify(msg);
    for (let ws of this.wss.clients) {
      ws.send(json);
    }
  }
}

function getSpecifier(dep: Dependency): string {
  if (typeof dep.meta.placeholder === 'string') {
    return dep.meta.placeholder;
  }

  return dep.specifier;
Repository: paritytech/substrate
File: core/network/src/util.rs (Rust)

Code before:

use linked_hash_set::LinkedHashSet;
use std::{hash::Hash, num::NonZeroUsize};

/// Wrapper around `LinkedHashSet` which grows bounded.
///
/// In the limit, for each element inserted the oldest existing element will be removed.
#[derive(Debug, Clone)]
pub(crate) struct LruHashSet<T: Hash + Eq> {
	set: LinkedHashSet<T>,
	limit: NonZeroUsize
}

impl<T: Hash + Eq> LruHashSet<T> {
	/// Create a new `LruHashSet` with the given limit.
	pub(crate) fn new(limit: NonZeroUsize) -> Self {
		Self { set: LinkedHashSet::new(), limit }
	}

	/// Insert element into the set.
	///
	/// Returns `true` if this is a new element to the set, `false` otherwise.
	/// Maintains the limit of the set by removing the oldest entry if necessary.
	pub(crate) fn insert(&mut self, e: T) -> bool {
		if self.set.len() == usize::from(self.limit) {
			if self.set.contains(&e) {
				return false
			}
			self.set.pop_front(); // remove oldest entry
		}
		self.set.insert(e)
	}
}

#[cfg(test)]
mod tests {
	use super::*;

	#[test]
	fn maintains_limit() {
		let two = NonZeroUsize::new(2).unwrap();
		let mut set = LruHashSet::new(two);

		// First element.
		assert!(set.insert(1u8));
		assert_eq!(1, set.set.len());
		assert!(set.set.contains(&1));

		// Inserting the same element multiple times has no effect.
		assert!(!set.insert(1u8));
		assert_eq!(1, set.set.len());

Review comment (suggestion):
I'm not sure about the runtime characteristics of the `contains` function, but we could always insert optimistically, and then `pop_front` if needed. This way this will have only one lookup.

Code after:

use linked_hash_set::LinkedHashSet;
use std::{hash::Hash, num::NonZeroUsize};

/// Wrapper around `LinkedHashSet` which grows bounded.
///
/// In the limit, for each element inserted the oldest existing element will be removed.
#[derive(Debug, Clone)]
pub(crate) struct LruHashSet<T: Hash + Eq> {
	set: LinkedHashSet<T>,
	limit: NonZeroUsize
}

impl<T: Hash + Eq> LruHashSet<T> {
	/// Create a new `LruHashSet` with the given (exclusive) limit.
	pub(crate) fn new(limit: NonZeroUsize) -> Self {
		Self { set: LinkedHashSet::new(), limit }
	}

	/// Insert element into the set.
	///
	/// Returns `true` if this is a new element to the set, `false` otherwise.
	/// Maintains the limit of the set by removing the oldest entry if necessary.
	/// Inserting the same element will update its LRU position.
	pub(crate) fn insert(&mut self, e: T) -> bool {
		if self.set.insert(e) {
			if self.set.len() == usize::from(self.limit) {
				self.set.pop_front(); // remove oldest entry
			}
			return true
		}
		false
	}
}

#[cfg(test)]
mod tests {
	use super::*;

	#[test]
	fn maintains_limit() {
		let three = NonZeroUsize::new(3).unwrap();
		let mut set = LruHashSet::<u8>::new(three);

		// First element.
		assert!(set.insert(1));
		assert_eq!(vec![&1], set.set.iter().collect::<Vec<_>>());

		// Second element.
		assert!(set.insert(2));
		assert_eq!(vec![&1, &2], set.set.iter().collect::<Vec<_>>());
Repository: paritytech/substrate
File: core/consensus/slots/src/aux_schema.rs (Rust)

Code before:

	pub fn snd_header(&self) -> &H {
		&self.snd_header
	}
}

/// Check if the header is an equivocation and returns the proof in that case.
/// Assumes all the headers in the same slot are signed by the same Signer.
pub fn check_equivocation<C, H, P>(
	backend: &Arc<C>,
	slot: u64,
	header: H,
	signer: P,
) -> ClientResult<Option<EquivocationProof<H>>>
	where
		H: Header,
		C: AuxStore,
		P: Encode + Decode + PartialEq,
{
	let mut key = SLOT_HEADER_MAP_KEY.to_vec();
	slot.using_encoded(|s| key.extend(s));

	let mut v = load_decode::<_, Vec<(H, P)>>(backend.clone(), &key[..])?
		.unwrap_or_else(Vec::new);

	for (prev_header, prev_signer) in v.iter() {
		if *prev_signer == signer {
			if header.hash() != prev_header.hash() {
				return Ok(Some(EquivocationProof {
					slot,
					fst_header: prev_header.clone(),
					snd_header: header.clone(),
				}));
			}
		}
	}

	// match slot_header_map.entry(slot) {
	// 	Entry::Vacant(vacant_entry) => {
	// 		vacant_entry.insert(vec![(header, signer)]);
	// 	},
	// 	Entry::Occupied(occupied_entry) => {
	// 		let v = occupied_entry.get();
	// 		for (prev_header, prev_signer) in v.iter() {
	// 			if *prev_signer == signer {
	// 				if header.hash() != prev_header.hash() {
	// 					return Ok(Some(EquivocationProof {
	// 						slot,
	// 						fst_header: prev_header.clone(),
	// 						snd_header: header.clone(),
	// 					}));

Review comment (performance):
This is `O(n)`, and is called `O(n)` times, so it is `O(n^2)` (where `n` is the number of messages from any particular validator).

Code after:
	/// Get the first header involved in the equivocation.
	pub fn fst_header(&self) -> &H {
		&self.fst_header
	}

	/// Get the second header involved in the equivocation.
	pub fn snd_header(&self) -> &H {
		&self.snd_header
	}
}

/// Checks if the header is an equivocation and returns the proof in that case.
///
/// Note: it detects equivocations only when slot_now - slot <= MAX_SLOT_CAPACITY.
pub fn check_equivocation<C, H, P>(
	backend: &Arc<C>,
	slot_now: u64,
	slot: u64,
	header: H,
	signer: P,
) -> ClientResult<Option<EquivocationProof<H>>>
	where
		H: Header,
		C: AuxStore,
		P: Encode + Decode + PartialEq,
{
	// We don't check equivocations for old headers out of our capacity.
	if slot_now - slot > MAX_SLOT_CAPACITY {
		return Ok(None)
	}

	// Key for this slot.
	let mut curr_slot_key = SLOT_HEADER_MAP_KEY.to_vec();
	slot.using_encoded(|s| curr_slot_key.extend(s));

	// Get headers of this slot.
	let mut headers_with_sig = load_decode::<_, Vec<(H, P)>>(backend.clone(), &curr_slot_key[..])?
		.unwrap_or_else(Vec::new);

	// Get first slot saved.
	let slot_header_start = SLOT_HEADER_START.to_vec();
	let first_saved_slot = load_decode::<_, u64>(backend.clone(), &slot_header_start[..])?
		.unwrap_or(slot);

	for (prev_header, prev_signer) in headers_with_sig.iter() {
		// A proof of equivocation consists of two headers:
		// 1) signed by the same voter,
		if *prev_signer == signer {
			// 2) with different hash
			if header.hash() != prev_header.hash() {
				return Ok(Some(EquivocationProof {
Repository: paritytech/substrate
File: core/consensus/aura/src/lib.rs (Rust)

Code before:
				slot_num,
				header.clone(),
				public.clone()
			) {
				Ok(Some(equivocation_proof)) => {
					info!(
						"Slot author {:?} is equivocating at slot {} with headers {:?} and {:?}",
						public,
						slot_num,
						equivocation_proof.fst_header().hash(),
						equivocation_proof.snd_header().hash(),
					);

					Err(format!(
						"Slot author {:?} is equivocating at slot {} with headers {:?} and {:?}",
						public,
						slot_num,
						equivocation_proof.fst_header().hash(),
						equivocation_proof.snd_header().hash(),
					))
				},
				Ok(None) => {
					Ok(CheckedHeader::Checked(header, digest_item))
				},
				Err(e) => {
					println!("{}", e.to_string());
					Err(e.to_string())
				},
			}
		} else {
			Err(format!("Bad signature on {:?}", hash))
		}
	}
}

/// A verifier for Aura blocks.
pub struct AuraVerifier<C, E, P> {
	client: Arc<C>,
	extra: E,
	phantom: PhantomData<P>,
	inherent_data_providers: inherents::InherentDataProviders,
	allow_old_seals: bool,
}

impl<C, E, P> AuraVerifier<C, E, P>
	where P: Send + Sync + 'static
{
	fn check_inherents<B: Block>(
		&self,
		block: B,
		block_id: BlockId<B>,

Review comment (suggestion):
This should be logged as `error!`, not written to `stdout`.

Code after:
				header.clone(),
				public.clone(),
			) {
				Ok(Some(equivocation_proof)) => {
					let log_str = format!(
						"Slot author is equivocating at slot {} with headers {:?} and {:?}",
						slot_num,
						equivocation_proof.fst_header().hash(),
						equivocation_proof.snd_header().hash(),
					);
					info!("{}", log_str);
					Err(log_str)
				},
				Ok(None) => Ok(CheckedHeader::Checked(header, digest_item)),
				Err(e) => Err(e.to_string()),
			}
		} else {
			Err(format!("Bad signature on {:?}", hash))
		}
	}
}

/// A verifier for Aura blocks.
pub struct AuraVerifier<C, E, P> {
	client: Arc<C>,
	extra: E,
	phantom: PhantomData<P>,
	inherent_data_providers: inherents::InherentDataProviders,
	allow_old_seals: bool,
}

impl<C, E, P> AuraVerifier<C, E, P>
	where P: Send + Sync + 'static
{
	fn check_inherents<B: Block>(
		&self,
		block: B,
		block_id: BlockId<B>,
		inherent_data: InherentData,
		timestamp_now: u64,
	) -> Result<(), String>
		where C: ProvideRuntimeApi, C::Api: BlockBuilderApi<B>
	{
		const MAX_TIMESTAMP_DRIFT_SECS: u64 = 60;

		let inherent_res = self.client.runtime_api().check_inherents(
			&block_id,
			block,
			inherent_data,
		).map_err(|e| format!("{:?}", e))?;
Repository: paritytech/substrate
File: core/consensus/slots/src/aux_schema.rs (Rust)

Code before:
	for (prev_header, prev_signer) in headers_with_sig.iter() {
		// A proof of equivocation consists of two headers:
		// 1) signed by the same voter,
		if *prev_signer == signer {
			// 2) with different hash
			if header.hash() != prev_header.hash() {
				return Ok(Some(EquivocationProof {
					slot, // 3) and mentioning the same slot.
					fst_header: prev_header.clone(),
					snd_header: header.clone(),
				}));
			} else {
				//  We don't need to continue in case of duplicated header,
				// since it's already saved and a possible equivocation
				// would have been detected before.
				return Ok(None)
			}
		}
	}

	let mut keys_to_delete = vec![];

	if slot_now % PRUNING_BOUND == 0 {
		let prefix = SLOT_HEADER_MAP_KEY.to_vec();

		let first_slot = slot_now - PRUNING_BOUND;
		let last_slot = slot_now - MAX_SLOT_CAPACITY;
		for s in first_slot..last_slot {
			let mut p = prefix.clone();
			s.using_encoded(|s| p.extend(s));
			keys_to_delete.push(p);
		}
	}

	headers_with_sig.push((header, signer));

	backend.insert_aux(
		&[(&curr_slot_key[..], headers_with_sig.encode().as_slice())],
		&keys_to_delete.iter().map(|k| &k[..]).collect::<Vec<&[u8]>>()[..],
	)?;

	Ok(None)
}


Review comment (suggestion):
```suggestion
		let first_slot = slot_now.saturating_sub(PRUNING_BOUND);
```

Code after:

	// Get headers of this slot.
	let mut headers_with_sig = load_decode::<_, Vec<(H, P)>>(backend.clone(), &curr_slot_key[..])?
		.unwrap_or_else(Vec::new);

	// Get first slot saved.
	let slot_header_start = SLOT_HEADER_START.to_vec();
	let first_saved_slot = load_decode::<_, u64>(backend.clone(), &slot_header_start[..])?
		.unwrap_or(slot);

	for (prev_header, prev_signer) in headers_with_sig.iter() {
		// A proof of equivocation consists of two headers:
		// 1) signed by the same voter,
		if *prev_signer == signer {
			// 2) with different hash
			if header.hash() != prev_header.hash() {
				return Ok(Some(EquivocationProof {
					slot, // 3) and mentioning the same slot.
					fst_header: prev_header.clone(),
					snd_header: header.clone(),
				}));
			} else {
				//  We don't need to continue in case of duplicated header,
				// since it's already saved and a possible equivocation
				// would have been detected before.
				return Ok(None)
			}
		}
	}

	let mut keys_to_delete = vec![];
	let mut new_first_saved_slot = first_saved_slot;

	if slot_now - first_saved_slot >= PRUNING_BOUND {
		let prefix = SLOT_HEADER_MAP_KEY.to_vec();
		new_first_saved_slot = slot_now.saturating_sub(MAX_SLOT_CAPACITY);

		for s in first_saved_slot..new_first_saved_slot {
			let mut p = prefix.clone();
			s.using_encoded(|s| p.extend(s));
			keys_to_delete.push(p);
		}
	}

	headers_with_sig.push((header, signer));

	backend.insert_aux(
		&[
			(&curr_slot_key[..], headers_with_sig.encode().as_slice()),
			(&slot_header_start[..], new_first_saved_slot.encode().as_slice()),
		],