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

Generalized parallel processing logic #148

Merged

Conversation

Baliedge
Copy link
Contributor

@Baliedge Baliedge commented Jul 27, 2023

Convert specialized parallel processing logic to generalized "translate" functions.
This is needed to prepare for significant datamodel changes supporting consistent ordering (#119) as the changes needed will refactor the same areas of code.
In several places, parallel processing has been implemented to improve performance. However, the logic is specialized and is prone to concurrency flaws without proper test coverage. Using a set of generalized functions will decrease the chance of introducing bugs when refactoring for consistent ordering. e.g. Want to be able to change the data type and container iteration logic without breaking channel logic.
Where possible, these translation functions will preserve input order.

Functions:

  • TranslateSliceParallel(): Iterates a slice, passes values through a translate function in parallel, then sends results sequentially in stable order to result function.
  • TranslateMapParallel(): Iterates a map, passes value pairs through a translate function in parallel, then sends results sequentially to result function. Ordering is nondeterministic due to nature of map built-in.
  • TranslatePipeline(): Reads an input channel, passes values through a translate function in parallel, then sends results sequentially to output channel in stable order.

@Baliedge Baliedge changed the title Generalized parallel processing logic WIP: Generalized parallel processing logic Jul 27, 2023
@Baliedge Baliedge force-pushed the Baliedge/PIP-2552-consistent-ordering-3 branch from f727a94 to 9bff565 Compare July 27, 2023 14:43
Copy link
Member

@daveshanley daveshanley left a comment

Choose a reason for hiding this comment

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

This is a great design! It standardizes async operations and keeps the speed, and introduces reliable ordering.

@Baliedge Baliedge marked this pull request as ready for review August 1, 2023 19:12
@Baliedge Baliedge changed the title WIP: Generalized parallel processing logic Generalized parallel processing logic Aug 1, 2023
@Baliedge Baliedge force-pushed the Baliedge/PIP-2552-consistent-ordering-3 branch 3 times, most recently from 53e3e31 to 8f710b2 Compare August 1, 2023 19:53
Copy link
Member

@daveshanley daveshanley left a comment

Choose a reason for hiding this comment

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

I've reviewed all the code, and I can't find anything wrong with it. It's better code than existed before. It's standardized async processing now vs. my custom code for each object type. It's easier to understand and easier to debug if needed.

This is great.

@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Patch coverage: 99.77% and no project coverage change.

Comparison is base (3c9415b) 99.80% compared to head (f95f6b4) 99.80%.

Additional details and impacted files
@@           Coverage Diff            @@
##             main     #148    +/-   ##
========================================
  Coverage   99.80%   99.80%            
========================================
  Files         148      149     +1     
  Lines       10673    10824   +151     
========================================
+ Hits        10652    10803   +151     
  Misses         21       21            
Flag Coverage Δ
unittests 99.80% <99.77%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
datamodel/low/v2/path_item.go 100.00% <ø> (ø)
datamodel/low/v3/paths.go 98.90% <98.21%> (+0.15%) ⬆️
datamodel/high/v2/path_item.go 100.00% <100.00%> (ø)
datamodel/high/v2/paths.go 100.00% <100.00%> (ø)
datamodel/high/v3/components.go 100.00% <100.00%> (ø)
datamodel/high/v3/paths.go 100.00% <100.00%> (ø)
datamodel/high/v3/responses.go 100.00% <100.00%> (ø)
datamodel/low/v2/paths.go 100.00% <100.00%> (ø)
datamodel/low/v3/components.go 100.00% <100.00%> (ø)
datamodel/low/v3/path_item.go 98.52% <100.00%> (-0.12%) ⬇️
... and 1 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Baliedge
Copy link
Contributor Author

Baliedge commented Aug 2, 2023

@daveshanley Ready for GHA re-run.

@daveshanley
Copy link
Member

There seems to be a flaky bit of code causing the tests to crash. It's caused by: TestPaths_Build_FailRefDeadEnd

https://github.com/pb33f/libopenapi/blob/main/datamodel/low/v3/paths_test.go#L122

It's crashing because circular detection is not being picked up and the stack is overflowing.

https://github.com/pb33f/libopenapi/blob/main/datamodel/low/extraction_functions.go#L79

Suggestion to fix is find out why the loop is stacking up out of control / put in a hard limit on the depth extractions can go, just as a hard limit on a runaway mine train. Anything more than 100 levels deep is way out of control anyway.

@Baliedge
Copy link
Contributor Author

@daveshanley FYI, been stuck on other projects lately. Maybe able to find time next week to resolve these test coverage issues.

@daveshanley
Copy link
Member

@daveshanley FYI, been stuck on other projects lately. Maybe able to find time next week to resolve these test coverage issues.

I hear you man, no problem at all. Quality takes time.

@daveshanley daveshanley mentioned this pull request Aug 28, 2023
@daveshanley
Copy link
Member

Some changes in my latest PR #162 should fix some of the issues causing the random failures.

@Baliedge Baliedge force-pushed the Baliedge/PIP-2552-consistent-ordering-3 branch from 4960be8 to df9f819 Compare August 29, 2023 18:13
@daveshanley
Copy link
Member

Just needs a tiny bit more coverage. Gotta keep it high.

@Baliedge Baliedge force-pushed the Baliedge/PIP-2552-consistent-ordering-3 branch from 21bf650 to 6aed734 Compare September 5, 2023 13:56
@daveshanley
Copy link
Member

I am OK with the coverage, this looks ready to merge. Are you ready?

@daveshanley
Copy link
Member

@Baliedge is this complete?

@Baliedge
Copy link
Contributor Author

@Baliedge is this complete?

Yes, it's functionally complete. The codecov isn't passing for small portions of uncovered error handlers. I haven't had the opportunity to address that yet.

@Baliedge
Copy link
Contributor Author

@daveshanley Test coverage should now be passing. Please re-run the PR checks.

…ize parallel processing of slices and channels in stable order.
Fix goroutine resource leak in `datamodel/low/v3/path_item.go`.
…ator.

Integrate `TranslateMapParallel()` into datamodel for `Paths` to replace specialized async logic.
@daveshanley
Copy link
Member

Which is less work to fix in a conflict, merging in #180 first, or this PR?

@daveshanley
Copy link
Member

Merging this first.

@daveshanley daveshanley merged commit 8b90e9a into pb33f:main Oct 5, 2023
2 of 3 checks passed
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