Skip to content

Conversation

@komyg
Copy link
Contributor

@komyg komyg commented Nov 11, 2025

No description provided.

@komyg komyg force-pushed the feature/promise.race branch from b1f57b2 to a204c21 Compare November 11, 2025 20:25
@komyg
Copy link
Contributor Author

komyg commented Nov 11, 2025

Hey @aapoalas, in this implementation of Promise.race I am using the infrastructure that we already have for PromiseGroup.

This means that we are creating a PromiseGroupRecord with all its elements, such as result_array and remaining_elements_count, which are not used by the Promise.race implementation:

pub struct PromiseGroupRecord<'a> {
    pub(crate) promise_group_type: PromiseGroupType,
    pub(crate) remaining_elements_count: u32,
    pub(crate) result_array: Array<'a>,
    pub(crate) promise: Promise<'a>,
}

We can keep it like this, or I can create a data structure specially for Promise.race that doesn't have any unused fields. This means also creating heap vectors to hold a PromiseRaceRecord struct. What do you prefer?

aapoalas
aapoalas previously approved these changes Nov 11, 2025
Copy link
Member

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

LGTM :)

@aapoalas
Copy link
Member

Hey @aapoalas, in this implementation of Promise.race I am using the infrastructure that we already have for PromiseGroup.

This means that we are creating a PromiseGroupRecord with all its elements, such as result_array and remaining_elements_count, which are not used by the Promise.race implementation:

pub struct PromiseGroupRecord<'a> {
    pub(crate) promise_group_type: PromiseGroupType,
    pub(crate) remaining_elements_count: u32,
    pub(crate) result_array: Array<'a>,
    pub(crate) promise: Promise<'a>,
}

We can keep it like this, or I can create a data structure specially for Promise.race that doesn't have any unused fields. This means also creating heap vectors to hold a PromiseRaceRecord struct. What do you prefer?

Hmm.... Do we actually need any kind of PromiseGroup at all...? Or can we just use the normal PromiseResolve stuff?

@komyg
Copy link
Contributor Author

komyg commented Nov 11, 2025

Hmm.... Do we actually need any kind of PromiseGroup at all...? Or can we just use the normal PromiseResolve stuff?

Well, I used the PromiseGroup, because the initial part of the spec is exactly the same for Promise.all, Promise.any, Promise.allSettled and Promise.race. Plus, all of them take in an iterable as an argument.

The difference is in the Perform Promise Race method, that doesn't use all the state that the other methods use. Even so, I am also using the immediately_resolve and immediatelly_reject functions from the PromiseGroup.

So if we can accept a slight more memory usage for Promise.race, I think we can continue as is.

As for the Promise.Resolve, I am not sure. The difference here is that Promise.race takes an array of promises, which is currently handled by PromiseGroup, and the Promise.Resolve will take a single value.

@komyg komyg requested a review from aapoalas November 11, 2025 23:14
@aapoalas
Copy link
Member

What I mean is that when you wrote your very first attempt at implementing Promise.all, you did it without any PromiseGroup or anything. You were simply chaining promises together using promise_inner_then or something like that. It seems that here, that would not only be functional but also exactly correct.

@komyg komyg force-pushed the feature/promise.race branch from 1b22da0 to 09c954e Compare November 14, 2025 12:58
@komyg
Copy link
Contributor Author

komyg commented Nov 14, 2025

What I mean is that when you wrote your very first attempt at implementing Promise.all, you did it without any PromiseGroup or anything. You were simply chaining promises together using promise_inner_then or something like that. It seems that here, that would not only be functional but also exactly correct.

I think I understand.

I made another attempt here: #889, without using the promise_group.

@komyg komyg closed this Nov 15, 2025
@komyg komyg deleted the feature/promise.race branch November 15, 2025 12:59
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.

2 participants