Skip to content
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

RC 1.0.5 #251

Merged
merged 12 commits into from
Oct 23, 2023
Merged

RC 1.0.5 #251

merged 12 commits into from
Oct 23, 2023

Conversation

EmilHvitfeldt
Copy link
Member

to close #250

@aitap
Copy link

aitap commented Oct 19, 2023

Your CPU time problems are likely caused by text2vec using parallel::mcparallel in addition to data.table. This definitely needs to be reported to the text2vec maintainer because of their default of parallel::detectCores(), which CRAN doesn't like any more. As a workaround, you can set options(text2vec.mc.cores=...) for the duration of the example.

@EmilHvitfeldt
Copy link
Member Author

Hello @aitap! Thanks for looking into this!

I tried that a while ago a7458a3

I don't think they are the issue as parallel::mcparallel is only enabled when using 20itoken_parallel which I don't do in this package https://github.com/search?q=repo:tidymodels/textrecipes%20itoken_parallel&type=code

Unless I need to do that in addition to the data.table stuff. let me try that

@aitap
Copy link

aitap commented Oct 19, 2023 via email

@aitap
Copy link

aitap commented Oct 20, 2023 via email

@EmilHvitfeldt
Copy link
Member Author

@aitap Thank you so much for looking into this! Would you be able to teach me what you did to gain those insights? I'm fine if you don't, you already helped me so much!

@EmilHvitfeldt EmilHvitfeldt merged commit 30e607a into main Oct 23, 2023
7 checks passed
@EmilHvitfeldt EmilHvitfeldt deleted the RC-1.0.5 branch October 23, 2023 00:50
@aitap
Copy link

aitap commented Oct 23, 2023

My first idea was that if something takes too much CPU time, it ought to show up in the CPU profiler. Writing R Extensions mentions a few profiling techniques. I've heard good things about Intel's VTune profiler (on both GNU/Linux and Windows) and Very Sleepy on Windows. The section on profiling in Algorithmica is a bit terse but contains useful pointers. I'm using perf on GNU/Linux because it's easy to install and doesn't require any instrumentation of the executables besides compiling with -g (which R already does). In the end, the traces I got for system.time(replicate(100, example(step_dummy_hash))) didn't give any obvious pointers, but I did consistently see 1.3 times of CPU time consumed per unit of real time. The CRAN machine has a different processor; maybe threads are more expensive in their circumstances?

I'm using an accelerated BLAS that can start threads on its own, so I was ready to blame OpenBLAS for the threads, but even with the environment variable OPENBLAS_NUM_THREADS=1 set I could see more than 100% of one CPU core being used (as measured by system.time).

If threads do evidently consume extra CPU time, perhaps some information could be gained by seeing who creates them? The OpenMP specification is not 100% precise on this, but it makes sense for a program to start new threads when it encounters a #pragma omp parallel directive and stop them when they are done. I've read Butenhof, 1997 and decided to set a breakpoint on pthread_create, the function used to start threads on POSIX-compatible operating systems. The OpenMP runtime implementors could have used a system-specific call (e.g. clone(2) on Linux), but they did use pthread_create, so the breakpoints fired in appropriate places.

The omp_thread_count() function was surprising because it looks like an OpenMP API but is not. The author could have used omp_get_thread_limit instead of creating a parallel section just to run omp_get_num_threads or counting threads one by one. The involvement of rsparse was not 100% conclusive (maybe there's a third package that also creates threads that I haven't noticed yet?), but it was definitely worth trying to limit the number of threads used by this package.

Copy link

github-actions bot commented Nov 7, 2023

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release textrecipes 1.0.5
2 participants