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

Reduce the number of times unix_time_update is called. #1057

Merged
merged 1 commit into from
Aug 17, 2018

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Aug 10, 2018

Reduced by, e.g.:

  • file_transfer_test: 33% of the clock_gettime calls.
  • tox_many_test: 53% of the clock_gettime calls.

Other tests will see similar improvements. Real world applications will
be closer to 40-50% improvement, since tox_many_test has 100 nodes, while
file_transfer_test has 2 nodes.


This change is Reviewable

@iphydf iphydf added this to the v0.2.x milestone Aug 10, 2018
@iphydf iphydf force-pushed the fewer-time-updates branch 6 times, most recently from 6161861 to 9777f65 Compare August 13, 2018 22:30
@zugz
Copy link

zugz commented Aug 14, 2018

What about testing/DHT_test.c?

Copy link

@zugz zugz left a comment

Choose a reason for hiding this comment

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

Shouldn't the time also be updated in testing/DHT_test.c, and in new_onions() in onion_test.c, and in test_add_to_list and test_list_main in auto_tests/dht_test.c?

Is it definitely ok that e.g. the timestamp associated to sending a packet is that of the start of the tox iteration, rather than the time the packet is actually sent?

How are writers of tests like these going to know when they should update the time?

@iphydf
Copy link
Member Author

iphydf commented Aug 14, 2018

Fixed DHT_test.c.

Time updates only need to be done when you are going to consume time. The dht tests you mentioned don't do anything related to time. Yes, new_onions may need that temporarily. I think it would make sense to call mono_time_update in mono_time_new so the time starts off being valid and doesn't need initialisation in various places. As for where to update it, that is indeed up to the test author, if they are going to test internals. Most auto-tests should be testing the public api and thus don't need to care about time keeping. It is already true today that test authors need to know when/whether to update times if they work with internals. If they don't know, their test may or may not work depending on where we have the time updates in our internals. This makes tests brittle, and makes internal refactoring harder.

@iphydf iphydf force-pushed the fewer-time-updates branch 3 times, most recently from e5c4cec to 24a9b97 Compare August 14, 2018 22:05
Reduced by, e.g.:
* `file_transfer_test`: 33% of the `clock_gettime` calls.
* `tox_many_test`: 53% of the `clock_gettime` calls.

Other tests will see similar improvements. Real world applications will
be closer to 40-50% improvement, since tox_many_test has 100 nodes, while
file_transfer_test has 2 nodes.
@iphydf iphydf merged commit 54066f3 into TokTok:master Aug 17, 2018
@iphydf iphydf deleted the fewer-time-updates branch August 17, 2018 22:14
@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.7 Aug 30, 2018
This pull request was closed.
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.

3 participants