search

radical-cybertools / radical.utils

Utility classes and tools for various radical projects
Other
8 stars 6 forks source link

Feature/fast ids #361

Closed andre-merzky closed 2 years ago

andre-merzky commented 2 years ago

Support for fast ID generation which provides process-local unique IDs w/o file access or system callouts

codecov[bot] commented 2 years ago

Codecov Report

Merging #361 (92b0d20) into devel (fafe2d4) will increase coverage by 0.09%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            devel     #361      +/-   ##
==========================================
+ Coverage   60.99%   61.09%   +0.09%     
==========================================
  Files          59       59              
  Lines        6623     6639      +16     
==========================================
+ Hits         4040     4056      +16     
  Misses       2583     2583
Impacted Files Coverage Δ
src/radical/utils/ids.py 94.63% <100.00%> (+0.64%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

mtitov commented 2 years ago

@andre-merzky isn't it the same as SIMPLE? btw, it makes sense to update current approaches as simple -> mem_counter / counter (and it could be enhanced with fast-approach) and item_counter -> file_counter

andre-merzky commented 2 years ago

@andre-merzky isn't it the same as SIMPLE? btw, it makes sense to update current approaches as simple -> mem_counter / counter (and it could be enhanced with fast-approach) and item_counter -> file_counter

I also wanted to avoid, most of all, the code paths we have in the 'proper' ID generation. FAST is 4 times faster than SIMPLE mainly for avoiding those. Adding a mem_counter and then using a single code path gives the same semantics, but not the same performance I'm afraid.

I stumbled over this when creating millions of tasks in a test. SIMPLE performs ok (about 200k / second), but FAST moved that to about 900k/sec - that happened to remove a bottleneck for that test I did :-)

mtitov commented 2 years ago

Then I would propose to change the current approach:

Proposed ID_FAST is the same as ID_SIMPLE, but without lock mechanism (do we need that?) - maybe we can have just one of them as ID_BASE. Just it seems that bringing up ID_FAST this way breaks the consistency of the module

p.s., as I see, ID_SIMPLE is used in SAGA only (within RADICAL packages)

andre-merzky commented 2 years ago

So instead of one special case we have 5 special cases? I don't think that improves things, really (and I also consider that refactoring out of scope for this PR). Hmmm...

mtitov commented 2 years ago

So instead of one special case we have 5 special cases? I don't think that improves things, really (and I also consider that refactoring out of scope for this PR).

Sorry, I disagree that it is out of scope.. while you introduce a new feature, you either follow the module design/approach, or you change it, so the new feature would fit (considering to continue support all existing cases). It seems that ID_FAST suppose to replace ID_SIMPLE (since why do we need the same functionality with two implementations?), but also the code is developed in a different way and I think it is reasonable to extend approach for a special case to other such cases (the general case - custom - suppose to reuse template parts from that special cases).

p.s. ID_FASTEST will be another new mode? ;)

(*) Have couple remarks to the current changes (will comment below), but my proposal is to rework this PR (as I wrote above)

andre-merzky commented 2 years ago

I don't think it's worth the effort. Was just response to a perf test I did, so not important enough....