Skip to content

feat: Implement promise as an environment for faster construction#191

Merged
schloerke merged 14 commits into
mainfrom
feat/env
Oct 22, 2025
Merged

feat: Implement promise as an environment for faster construction#191
schloerke merged 14 commits into
mainfrom
feat/env

Conversation

@shikokuchuo

Copy link
Copy Markdown
Member

Completes #169 by @schloerke with the help of Claude Code.

Our Promise class does not utilize many of R6's features. From a conversation with Will Landau, I was curious to see if we could make an environment instead of an R6 class.

The only addition here is to make private accessible from self (mimicking R6 internals) so that we don't break code/packages which reach into Promise internals. This would include my PR #190.

I've run revdepchecks and they come in clean.

n <- 100 * 1000

profvis::profvis({
  message("Create:")
  # print(system.time({
  proms <- lapply(seq_len(n), promise_resolve)
  p <- promise_all(.list = proms)
  # }))
  message("Execute:")
  # print(system.time({
  while (!later::loop_empty()) {
    later::run_now()
  }
  # }))
  message("Done!")
})

The above profiling consistently returns in 10-12s on my Macbook compared to 16-18s previously. This suggests that the optimization is not insignificant. @wlandau you're also welcome to comment based on your real life usage.

@schloerke

schloerke commented Oct 21, 2025

Copy link
Copy Markdown
Contributor

A 10x improvement in creation!

PR:

❯❯ bench::mark(Promise())
# A tibble: 1 × 13
  expression     min median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result    memory     time       gc      
  <bch:expr> <bch:t> <bch:>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list>    <list>     <list>     <list>  
1 Promise()   2.01µs 2.67µs   348994.      560B        0 10000     0     28.7ms <Promise> <Rprofmem> <bench_tm> <tibble>

main:

❯❯ bench::mark(Promise$new())
# A tibble: 1 × 13
  expression     min median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result    memory     time       gc      
  <bch:expr>  <bch:> <bch:>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list>    <list>     <list>     <list>  
1 Promise$ne… 24.7µs 26.7µs    33420.        0B     43.5  9987    13      299ms <Promise> <Rprofmem> <bench_tm> <tibble>

I adjusted the earlier profiling to not include promise_all as that was it's own bug at the time.

n <- 100 * 1000

profvis::profvis({
  message("Create")
  proms <- lapply(seq_len(n), promise_resolve)

  message("Execute")
  while (!later::loop_empty()) {
    later::run_now()
  }
  message("Done!")
})

main is ~8s. PR is ~3-4s. 🥳

@schloerke schloerke left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you!

@schloerke

Copy link
Copy Markdown
Contributor

Am going to merge to let shinycoreci run later tonight.

@schloerke schloerke merged commit 82cf56f into main Oct 22, 2025
21 checks passed
@schloerke schloerke deleted the feat/env branch October 22, 2025 20:31
@wch

wch commented Oct 24, 2025

Copy link
Copy Markdown
Collaborator

I just took a quick look. I think in general it can make sense to replace R6 classes with environments when speed matters. I've done that a number of times myself, for example in fastmap: https://github.com/r-lib/fastmap/blob/main/R/fastmap.R

In the case of promises, I think probably the biggest challenge is if someone has code that uses inheritance from promise objects. I think it would not be feasible to get inheritance working with R6-but-not-actually-R6 classes.

For measuring performance, I suggest also benchmarking the overhead of S3 dispatch from $.promise, and various other S3 generics that promises are often used with. IIRC, S3 dispatch is surprisingly expensive -- that's why in fastmap, the object that's returned doesn't have a class attribute on it. I say this not because it makes sense to remove class from promise objects (that would break a lot of stuff), but because it's worth looking at the promise creation time in context of everything else that is done with promises.

I have a feeling that if you profile promises in real-world use, the time spent in promise creation will barely show up compared to other things in the lifecycle of a promise -- but I don't know for sure, so that's why it's worth getting data on it.

@wch

wch commented Oct 24, 2025

Copy link
Copy Markdown
Collaborator

I realized that the Promise class isn't exported, so inheritance shouldn't be a problem. So with that out of the way, I think this is a very good change!

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.

3 participants