You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jonkeane (via GitHub)" <gi...@apache.org> on 2025/12/04 20:56:36 UTC

[PR] GH-48340: [R] respected `MAKEFLAGS` [arrow]

jonkeane opened a new pull request, #48341:
URL: https://github.com/apache/arrow/pull/48341

   ### Rationale for this change
   Respect the `MAKEFLAGS` that someone has set.
   
   Resolves #48340
   
   ### What changes are included in this PR?
   Respect
   
   ### Are these changes tested?
   No, though we should see speedups in various places.
   
   ### Are there any user-facing changes?
   More respect.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-48340: [R] respected `MAKEFLAGS` [arrow]

Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on PR #48341:
URL: https://github.com/apache/arrow/pull/48341#issuecomment-3627191535

   I'm not finding anything besides full parallelism, and we need to keep our default limit for CRAN builders, so I'm going to merge this, but if we find a better way in the future, I'm all for making this (more) standard (c)make.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-48340: [R] respected `MAKEFLAGS` [arrow]

Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on PR #48341:
URL: https://github.com/apache/arrow/pull/48341#issuecomment-3617287441

   > You've confirmed that this does the expected thing?
   
   Yeah, the output to the line `${CMAKE} --build . --target install -- -j $N_JOBS` there is quite obvious. 
   
   Interestingly — on my machine moving with the respecting versus without wasn't vastly different. Going from 2 to 12 it was on the order of a 2x faster. But that's probably not totally surprising given that there's other overhead going on.
   
    > I have a hard time believing that this is the canonical way to figure out how many workers make will use--there's no way to query make for its config? But I guess this is fine?
    
    Hmmm lemme see if I can find something. The stuff I've found so far would enable full parallelism. But if not, I agree this is fine for now.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-48340: [R] respected `MAKEFLAGS` [arrow]

Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on code in PR #48341:
URL: https://github.com/apache/arrow/pull/48341#discussion_r2590558435


##########
r/tools/nixlibs.R:
##########
@@ -540,6 +540,12 @@ build_libarrow <- function(src_dir, dst_dir) {
   if (makeflags == "") {
     makeflags <- sprintf("-j%s", ncores)
     Sys.setenv(MAKEFLAGS = makeflags)
+  } else {
+    # Extract -j value from existing MAKEFLAGS if present
+    j_match <- regmatches(makeflags, regexpr("-j\\s*([0-9]+)", makeflags, perl = TRUE))
+    if (length(j_match) > 0) {
+      ncores <- as.integer(sub("-j\\s*", "", j_match, perl = TRUE))
+    }

Review Comment:
   Alternatively, we could probably pass our `MAKEFLAGS` all the way down to https://github.com/apache/arrow/blob/2360a0c82c89581bbc9bc669544b24c0c57ea886/r/inst/build_arrow_static.sh#L112 and then paste them there instead of the `N_JOBS` bit.
   
   Though this is more regexful, it is slightly less passing of large(r) things around so I went this route.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-48340: [R] respected `MAKEFLAGS` [arrow]

Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #48341:
URL: https://github.com/apache/arrow/pull/48341#issuecomment-3677934777

   After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 0f51aba2f354f8213bebc9125588c550a6ef8de8.
   
   There were no benchmark performance regressions. 🎉
   
   The [full Conbench report](https://github.com/apache/arrow/runs/58613820820) has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org