Fine-tune the Ordering for the atomic usages

[wang384670111]

SeqCst is overly restrictive. I believe that the ordering can be appropriately modified.

In messages model, I believe that MESSAGES, IGNORE_MESSAGES, and ERRORED are merely signals for multithreading purposes and do not synchronize with other locals. Therefore, using Relaxed ordering should suffice.

In lib and utils models, COUNTER and NEXT_ID are used for multithreading counting purposes, so using Relaxed ordering is sufficient.

In walk model, the load and store of quit_now should use Acquire/Release to ensure that the stack is up-to-date.

[BurntSushi]

I used SeqCst in part because I didn't want to do the analysis necessary to demonstrate that a less restrictive ordering was correct, and also because I didn't perceive using SeqCst to be a particular issue in most of these cases.

With that said, I buy all of the changes to Relaxed here I think.

However, I'm unsure of the Require/Release changes in the ignore crate. I think those warrant comments explaining their correctness.

[BurntSushi]

I think this and the Release above need comments justifying why this is correct.

[wang384670111]

I used SeqCst in part because I didn't want to do the analysis necessary to demonstrate that a less restrictive ordering was correct, and also because I didn't perceive using SeqCst to be a particular issue in most of these cases.

With that said, I buy all of the changes to Relaxed here I think.

However, I'm unsure of the Require/Release changes in the ignore crate. I think those warrant comments explaining their correctness.

My reasoning is based on the following observations:

Line 1501 calls quit_now after calling run_one at line 1562, where run_one invokes generate_work. Inside generate_work, specifically at line 1655, the send performs a push on the stack.

ripgrep/crates/ignore/src/walk.rs

Lines 1498 to 1504 in 648a65f

	fn run(mut self) {
	while let Some(work) = self.get_work() {
	if let WalkState::Quit = self.run_one(work) {
	self.quit_now();
	}
	}
	}

ripgrep/crates/ignore/src/walk.rs

Lines 1654 to 1656 in 648a65f

	if !should_skip_filesize && !should_skip_filtered {
	self.send(Work { dent, ignore: ig.clone(), root_device });
	}

Then, after executing is_quit_now at line 1669, send_quit is called at line 1680 to access the stack and perform another push.

ripgrep/crates/ignore/src/walk.rs

Lines 1669 to 1682 in 648a65f

	if self.is_quit_now() {
	value = Some(Message::Quit)
	}
	match value {
	Some(Message::Work(work)) => {
	return Some(work);
	}
	Some(Message::Quit) => {
	// Repeat quit message to wake up sleeping threads, if
	// any. The domino effect will ensure that every thread
	// will quit.
	self.send_quit();
	return None;
	}

Based on this sequence of operations, in the absence of a definitive happen-before relationship within a concurrent environment, I believe it is necessary to use Acquire/Release to establish synchronization, in order to properly observe these modifications to stack.

[wang384670111]

I used SeqCst in part because I didn't want to do the analysis necessary to demonstrate that a less restrictive ordering was correct, and also because I didn't perceive using SeqCst to be a particular issue in most of these cases.
With that said, I buy all of the changes to Relaxed here I think.
However, I'm unsure of the Require/Release changes in the ignore crate. I think those warrant comments explaining their correctness.

My reasoning is based on the following observations:

Line 1501 calls quit_now after calling run_one at line 1562, where run_one invokes generate_work. Inside generate_work, specifically at line 1655, the send performs a push on the stack.

ripgrep/crates/ignore/src/walk.rs

Lines 1498 to 1504 in 648a65f

	fn run(mut self) {
	while let Some(work) = self.get_work() {
	if let WalkState::Quit = self.run_one(work) {
	self.quit_now();
	}
	}
	}

ripgrep/crates/ignore/src/walk.rs

Lines 1654 to 1656 in 648a65f

	if !should_skip_filesize && !should_skip_filtered {
	self.send(Work { dent, ignore: ig.clone(), root_device });
	}

Then, after executing is_quit_now at line 1669, send_quit is called at line 1680 to access the stack and perform another push.

ripgrep/crates/ignore/src/walk.rs

Lines 1669 to 1682 in 648a65f

	if self.is_quit_now() {
	value = Some(Message::Quit)
	}
	match value {
	Some(Message::Work(work)) => {
	return Some(work);
	}
	Some(Message::Quit) => {
	// Repeat quit message to wake up sleeping threads, if
	// any. The domino effect will ensure that every thread
	// will quit.
	self.send_quit();
	return None;
	}

Based on this sequence of operations, in the absence of a definitive happen-before relationship within a concurrent environment, I believe it is necessary to use Acquire/Release to establish synchronization, in order to properly observe these modifications to stack.

I used SeqCst in part because I didn't want to do the analysis necessary to demonstrate that a less restrictive ordering was correct, and also because I didn't perceive using SeqCst to be a particular issue in most of these cases.
With that said, I buy all of the changes to Relaxed here I think.
However, I'm unsure of the Require/Release changes in the ignore crate. I think those warrant comments explaining their correctness.

My reasoning is based on the following observations:

Line 1501 calls quit_now after calling run_one at line 1562, where run_one invokes generate_work. Inside generate_work, specifically at line 1655, the send performs a push on the stack.

ripgrep/crates/ignore/src/walk.rs

Lines 1498 to 1504 in 648a65f

	fn run(mut self) {
	while let Some(work) = self.get_work() {
	if let WalkState::Quit = self.run_one(work) {
	self.quit_now();
	}
	}
	}

ripgrep/crates/ignore/src/walk.rs

Lines 1654 to 1656 in 648a65f

	if !should_skip_filesize && !should_skip_filtered {
	self.send(Work { dent, ignore: ig.clone(), root_device });
	}

Then, after executing is_quit_now at line 1669, send_quit is called at line 1680 to access the stack and perform another push.

ripgrep/crates/ignore/src/walk.rs

Lines 1669 to 1682 in 648a65f

	if self.is_quit_now() {
	value = Some(Message::Quit)
	}
	match value {
	Some(Message::Work(work)) => {
	return Some(work);
	}
	Some(Message::Quit) => {
	// Repeat quit message to wake up sleeping threads, if
	// any. The domino effect will ensure that every thread
	// will quit.
	self.send_quit();
	return None;
	}

Based on this sequence of operations, in the absence of a definitive happen-before relationship within a concurrent environment, I believe it is necessary to use Acquire/Release to establish synchronization, in order to properly observe these modifications to stack.

I've reconsidered my earlier deduction and realized I overlooked a crucial detail.

Before calling quit_now at 1501, we have a successful return of WalkState::Quit at 1500. This indicates that the stack push operation, occurring at line 1655, has completed. This establishes a happen-before relationship, making it unnecessary to use Release in quit_now to ensure the completion of the stack push operation. Therefore, using Relaxed for both the store in quit_now and the subsequent load should suffice.

I would greatly appreciate your feedback on this observation. I'm eager for the potential merge and am ready to provide any further clarifications as needed.

[BurntSushi]

It might take me some time to look into this. I basically need to convince myself of your argument.

If you want to get something merged more quickly, you could open a new PR with the less "interesting" changes and keep this PR focused on the changes to ignore's parallel traversal.