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.

Sign up for GitHub

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

Jump to bottom

READY: lists/skiplists/rb-trees new API & sequence lock & atomic lists #3045

Merged
merged 21 commits into from
Apr 30, 2019
Merged

READY: lists/skiplists/rb-trees new API & sequence lock & atomic lists #3045

merged 21 commits into from
Apr 30, 2019

Conversation

eqvinox
Copy link
Contributor

@eqvinox eqvinox commented Sep 18, 2018

Had this stuff on my disk for 2~3 years now, finally completed the unit tests now. Still needs proper docs, but comments welcome.

@NetDEF-CI

This comment has been minimized.

Sign in to view
@eqvinox

This comment has been minimized.

Sign in to view
@NetDEF-CI

This comment has been minimized.

Sign in to view
@LabN-CI

This comment has been minimized.

Sign in to view
@NetDEF-CI

This comment has been minimized.

Sign in to view
@NetDEF-CI

This comment has been minimized.

Sign in to view
@NetDEF-CI

This comment has been minimized.

Sign in to view
@LabN-CI

This comment has been minimized.

Sign in to view
@LabN-CI

This comment has been minimized.

Sign in to view
@NetDEF-CI

This comment has been minimized.

Sign in to view
@LabN-CI

This comment has been minimized.

Sign in to view
@NetDEF-CI

This comment has been minimized.

Sign in to view
@donaldsharp
Copy link
Member

so what's the difference between the linux futex call and the sys_futex call added in lib/seqlock.c? Other than that I don't see any problems here other than the need for documentation on what you are doing and where you are going :)

@eqvinox eqvinox added the submitter action required The author/submitter needs to do something (fix, rebase, add info, etc.) label Oct 23, 2018
@eqvinox
Copy link
Contributor Author

eqvinox commented Nov 13, 2018

Some docs added.

futex() is a wrapper for SYS_futex since the latter is not exposed by glibc.

@LabN-CI

This comment has been minimized.

Sign in to view
@NetDEF-CI

This comment has been minimized.

Sign in to view
eqvinox added 3 commits April 27, 2019 19:33
@eqvinox
bgpd: replace ADV_FIFO with DECLARE_LIST
a274fef 
The FIFO_* stuff in lib/fifo.h is no different from a simple unsorted
list.  Just use DECLARE_LIST here so we can get rid of FIFO_*.

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
@eqvinox
bgpd: replace label pool fifo with DECLARE_LIST
41397f2 
Again, the FIFO_* stuff in lib/fifo.h is no different from a simple
unsorted list.  Just use DECLARE_LIST here so we can get rid of FIFO_*.

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
@eqvinox
lib: remove fifo implementation
a297301
@LabN-CI

This comment has been minimized.

Sign in to view
@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7394/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

CLANG Static Analyzer Summary

  • Github Pull Request 3045, comparing to Git base SHA 86336f6

No Changes in Static Analysis warnings compared to base

12 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7394/artifact/shared/static_analysis/index.html

@eqvinox eqvinox added review & merge me look at me! and removed submitter action required The author/submitter needs to do something (fix, rebase, add info, etc.) labels Apr 27, 2019
@eqvinox
Copy link
Contributor Author

eqvinox commented Apr 27, 2019

Probably needs a search&replace in @louberger 's setup

@louberger
Copy link
Member

louberger commented Apr 27, 2019 via email

louberger
louberger reviewed Apr 28, 2019
View reviewed changes
lib/atomlist.c
@@ -0,0 +1,348 @@
/*
* Copyright (c) 2016-2018 David Lamparter, for NetDEF, Inc.
*
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to clearly delineate who the copyright holder is here e.g., is it
NetDef (with Author being David)
or
David (with an acknowledgement to netdef for supporting the work)

@louberger
Copy link
Member

louberger commented Apr 28, 2019 via email

@LabN-CI
Copy link
Collaborator

LabN-CI commented Apr 28, 2019

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/3045 a297301
Date 04/28/2019
Start 16:56:51
Finish 17:22:02
Run-Time 25:11
Total 1813
Pass 1813
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2019-04-28-16:56:51.txt
Log autoscript-2019-04-28-16:57:37.log.bz2
Memory 450 450 374

For details, please contact louberger

@louberger
Copy link
Member

I believe I'm seeing slower performance with this PR on some scaling tests. Will rerun to see if just a test artefact...

@louberger
Copy link
Member

VNC time test:
6.17 VNC time test -- 50000 prefixes, 2 NVEs with 2 Servers
Master:
6.17.21 7.0.0.132 Total elapsed time=33.069 secs
This PR
6.17.21 7.0.0.132 Total elapsed time=82.47 secs

@LabN-CI
Copy link
Collaborator

LabN-CI commented Apr 29, 2019

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/3045 a297301
Date 04/28/2019
Start 20:21:54
Finish 20:46:01
Run-Time 24:07
Total 1813
Pass 1813
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2019-04-28-20:21:54.txt
Log autoscript-2019-04-28-20:22:42.log.bz2
Memory 444 446 374

For details, please contact louberger

@louberger
Copy link
Member

Rerun had better result:
6.17.21 7.0.0.132 Total elapsed time=48.782 secs

Does anyone have any good / other scali g tests to run for comparison?

@qlyoung qlyoung mentioned this pull request Apr 29, 2019
Closed
@eqvinox
Copy link
Contributor Author

eqvinox commented Apr 29, 2019

@louberger I pushed 2 reverts to try and identify where the slowdown is coming from. Can you rerun and see what the numbers are?

@eqvinox
Copy link
Contributor Author

eqvinox commented Apr 29, 2019

That said, why is there such a big difference between the first run and the rerun? That looks incredibly weird...

@LabN-CI
Copy link
Collaborator

LabN-CI commented Apr 29, 2019

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/3045 8390828
Date 04/29/2019
Start 15:21:54
Finish 15:45:51
Run-Time 23:57
Total 1816
Pass 1816
Fail 0
Valgrind-Errors 0
Valgrind-Loss 0
Details vncregress-2019-04-29-15:21:54.txt
Log autoscript-2019-04-29-15:22:39.log.bz2
Memory 439 446 374

For details, please contact louberger

@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7404/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

CLANG Static Analyzer Summary

  • Github Pull Request 3045, comparing to Git base SHA 86336f6

No Changes in Static Analysis warnings compared to base

12 Static Analyzer issues remaining.

See details at
https://ci1.netdef.org/browse/FRR-FRRPULLREQ-7404/artifact/shared/static_analysis/index.html

@louberger
Copy link
Member

Overall run time looks better, see above. Also:

6.17.21 7.0.0.132 Total elapsed time=45.032 secs

donaldsharp
donaldsharp approved these changes Apr 30, 2019
View reviewed changes
Copy link
Member

@donaldsharp donaldsharp left a comment

Choose a reason for hiding this comment

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

let's do this

@louberger louberger merged commit 31e944a into FRRouting:master Apr 30, 2019
eqvinox added a commit to opensourcerouting/frr that referenced this pull request Apr 30, 2019
@eqvinox
lib: revert datastructure reverts...
4cb91dc 
This restores the full changeset from FRRouting#3045, by reverting reverts:

- Revert "lib: use DECLARE_SKIPLIST for timers instead of pqueue"
- Revert "lib: remove pqueue_*"

Signed-off-by: David Lamparter <equinox@opensourcerouting.org>
@eqvinox eqvinox mentioned this pull request Apr 30, 2019
Closed
@eqvinox eqvinox deleted the atoms branch April 18, 2021 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@louberger louberger louberger left review comments

@donaldsharp donaldsharp donaldsharp approved these changes

@qlyoung qlyoung Awaiting requested review from qlyoung

Assignees
No one assigned
Labels
libfrr review & merge me look at me!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@eqvinox @NetDEF-CI @LabN-CI @donaldsharp @louberger @qlyoung