-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Prep some things for threading #1837
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
Conversation
Looks like |
d0932a6
to
2febbe1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work. Add some comments in regard to thread safety.
5006d25
to
1be3f0b
Compare
vm/src/obj/objiter.rs
Outdated
} | ||
Err(ref e) if objtype::isinstance(&e, &vm.ctx.exceptions.index_error) => { | ||
Err(new_stop_iteration(vm)) | ||
let mut prev = self.position.load(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something but I think we can go for a simpler solution here:
let step = if self.reversed { -1 } else { 1 };
let curr_pos = self.position.fetch_add(step);
if curr_pos >= 0 {
...
then use the same code as before.
This will answer what we expect from an iterator. Each time next is called we will receive a unique item from the iterable. Once the iterator is exhausted all consecutive calls will get stop iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that probably would work since it's an isize and it wouldn't matter if it went below 0, it wouldn't underflow.
vm/src/obj/objtuple.rs
Outdated
let ret = self.tuple.as_slice()[self.position.get()].clone(); | ||
self.position.set(self.position.get() + 1); | ||
Ok(ret) | ||
let pos = self.position.load(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again lets use fetch_add
here. In this solution we can have 2 thread getting the same value.
vm/src/util.rs
Outdated
done: impl Fn(usize) -> bool, | ||
next: impl Fn(usize) -> usize, | ||
) -> Option<usize> { | ||
let mut prev = pos.load(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comment in objiter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a comment on your last set of reviews; if it's a reverse iterator and reaches the end, then fetch_sub
would underflow and if next(it)
is called again the index would be usize::MAX_VALUE
. What would you suggest for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a separate done
flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do prefer to keep this simple as the current implementation is a little complex. I think we an go in a few directions here:
- Change the position to
isize
. - Start the position at
len(iterator)
instead oflen(iterator) - 1
and go until 0. - Check if we are at 0 before doing the
fetch_sub
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right, it'd be fine if we do a load and then a fetch_sub after we check that we're still in bounds, since it's still atomic. Okay, thanks for your feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this again I am not sure 2 or 3 will be simple at all. I think changing to isize
will be the simplest. You can't actually do load
and then fetch_sub
as it will not be atomic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I updated it to just use the fetch_*
operations. I didn't really want to do that, but I think I was just being silly, because yes, if someone does this:
it = iter([1,2])
try:
next(it)
except:
pass
Then it's eventually going to wrap around and start yielding elements again, but that's a very small edge case that doesn't really matter in the grand scheme of things.
4eed7f9
to
11f96b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few tests failing but once they are fixed we can merge this.
7fe9fab
to
15e31f4
Compare
15e31f4
to
0d0e973
Compare
0d0e973
to
a10936c
Compare
Related to #1831