index: Change sync variables to use std::chrono::steady_clock #25079

pull w0xlt wants to merge 1 commits into bitcoin:master from w0xlt:base_index_chrono changing 1 files +5 −5
  1. w0xlt commented at 6:25 pm on May 6, 2022: contributor
    This PR refactors the sync variables to use std::chrono::steady_clock as it is best suitable for measuring intervals.
  2. w0xlt renamed this:
    index: Change sync interval variables to std::chrono::seconds
    index, refactor: Change sync interval variables to std::chrono::seconds
    on May 6, 2022
  3. DrahtBot added the label UTXO Db and Indexes on May 6, 2022
  4. fanquake requested review from MarcoFalke on May 6, 2022
  5. fanquake requested review from mzumsande on May 6, 2022
  6. in src/index/base.cpp:163 in 98970885a4 outdated 
    159@@ -160,7 +160,7 @@ void BaseIndex::ThreadSync()
160                 pindex = pindex_next;
161             }
162 
163-            int64_t current_time = GetTime();
164+            std::chrono::seconds current_time = GetTime<std::chrono::seconds>();
    jonatack commented at 10:02 pm on May 6, 2022: 
    0            auto current_time{GetTime<std::chrono::seconds>()};

    It is clear from the rhs what the type is.

    w0xlt commented at 4:38 am on May 7, 2022:
    Done in https://github.com/bitcoin/bitcoin/pull/25079/commits/626b636b193baae81a697febd41a712f94a8a702. Thanks for the suggestion.
  7. in src/index/base.cpp:22 in 98970885a4 outdated 
    17@@ -18,8 +18,8 @@ using node::ReadBlockFromDisk;
18 
19 constexpr uint8_t DB_BEST_BLOCK{'B'};
20 
21-constexpr int64_t SYNC_LOG_INTERVAL = 30; // seconds
22-constexpr int64_t SYNC_LOCATOR_WRITE_INTERVAL = 30; // seconds
23+constexpr std::chrono::seconds SYNC_LOG_INTERVAL{30};
24+constexpr std::chrono::seconds SYNC_LOCATOR_WRITE_INTERVAL{30};
    jonatack commented at 10:03 pm on May 6, 2022:

    here and the line above and lines 133-134, can use std::chrono_literals

     0@@ -18,8 +18,8 @@ using node::ReadBlockFromDisk;
 1 
 2 constexpr uint8_t DB_BEST_BLOCK{'B'};
 3 
 4-constexpr std::chrono::seconds SYNC_LOG_INTERVAL{30};
 5-constexpr std::chrono::seconds SYNC_LOCATOR_WRITE_INTERVAL{30};
 6+constexpr auto SYNC_LOG_INTERVAL{30s};
 7+constexpr auto SYNC_LOCATOR_WRITE_INTERVAL{30s};
 8 
 9 template <typename... Args>
10 static void FatalError(const char* fmt, const Args&... args)
11@@ -130,8 +130,8 @@ void BaseIndex::ThreadSync()
12     if (!m_synced) {
13         auto& consensus_params = Params().GetConsensus();
14 
15-        std::chrono::seconds last_log_time{0};
16-        std::chrono::seconds last_locator_write_time{0};
17+        auto last_log_time{0s};
18+        auto last_locator_write_time{0s};
    w0xlt commented at 4:38 am on May 7, 2022:
    Done in https://github.com/bitcoin/bitcoin/pull/25079/commits/626b636b193baae81a697febd41a712f94a8a702. Thanks for the suggestion.
  8. jonatack commented at 10:10 pm on May 6, 2022: member
    ACK, some suggestions
  9. w0xlt force-pushed on May 7, 2022
  10. w0xlt force-pushed on May 7, 2022
  11. in src/index/base.cpp:163 in 626b636b19 outdated 
    159@@ -160,7 +160,7 @@ void BaseIndex::ThreadSync()
160                 pindex = pindex_next;
161             }
162 
163-            int64_t current_time = GetTime();
164+            auto current_time{GetTime<std::chrono::seconds>()};
    MarcoFalke commented at 5:32 am on May 7, 2022:

    given that the clock is used to schedule things, an alternative would be to change this to a steady clock. This will behave better if the system clock is adjusted. However std::chrono::steady_clock is not mockable, which I think is fine in this context here.

    0            auto current_time{std::chrono::steady_clock::now()};
    w0xlt commented at 6:01 am on May 7, 2022:

    steady_clock::now() returns steady_clock::time_point and this results in compilation error in the next line (last_log_time + SYNC_LOG_INTERVAL < current_time - no operator "<" matches these operands).

    With duration_cast, it seems to work.

    0- auto current_time{GetTime<std::chrono::seconds>()};
1+ auto current_time{std::chrono::duration_cast<std::chrono::seconds>(std::chrono::steady_clock::now().time_since_epoch())};
    MarcoFalke commented at 6:08 am on May 7, 2022:

    last_log_time should be of the same type.

    std::chrono::stead_clock::time_point

    w0xlt commented at 2:20 pm on May 7, 2022:
    I’m not very familiar with the std::chrono::steady_clock interface but I think https://github.com/bitcoin/bitcoin/pull/25079/commits/74a70ebea07ccbf5c7402f932b1880c499dd3ac0 does the trick.
  12. w0xlt force-pushed on May 7, 2022
  13. in src/index/base.cpp:22 in 74a70ebea0 outdated 
    17@@ -18,8 +18,8 @@ using node::ReadBlockFromDisk;
18 
19 constexpr uint8_t DB_BEST_BLOCK{'B'};
20 
21-constexpr int64_t SYNC_LOG_INTERVAL = 30; // seconds
22-constexpr int64_t SYNC_LOCATOR_WRITE_INTERVAL = 30; // seconds
23+constexpr std::chrono::steady_clock::duration SYNC_LOG_INTERVAL{30s};
24+constexpr std::chrono::steady_clock::duration SYNC_LOCATOR_WRITE_INTERVAL{30s};
    MarcoFalke commented at 3:46 pm on May 7, 2022:
    This can stay auto
  14. w0xlt force-pushed on May 8, 2022
  15. MarcoFalke commented at 7:01 am on May 8, 2022: member
    You’ll also have to change the title to explain the behavior change
  16. index, refactor: Change sync variables to use `std::chrono::steady_clock` 
    This refactors the sync variables to use `std::chrono::steady_clock`
as it is best suitable for measuring intervals.
    92b35aba22
  17. w0xlt renamed this:
    index, refactor: Change sync interval variables to std::chrono::seconds
    index, refactor: Change sync variables to use `std::chrono::steady_clock`
    on May 8, 2022
  18. w0xlt force-pushed on May 8, 2022
  19. w0xlt commented at 7:11 am on May 8, 2022: contributor
    Done in https://github.com/bitcoin/bitcoin/pull/25079/commits/92b35aba224ad4440f3ea6c01c841596a6a3d6f4. Also changed the PR title and description.
  20. jonatack commented at 7:56 am on May 8, 2022: member
    utACK 92b35aba224ad4440f3ea6c01c841596a6a3d6f4
  21. MarcoFalke renamed this:
    index, refactor: Change sync variables to use `std::chrono::steady_clock`
    index: Change sync variables to use `std::chrono::steady_clock`
    on May 8, 2022
  22. MarcoFalke requested review from ajtowns on May 8, 2022
  23. ajtowns commented at 9:01 pm on May 9, 2022: member
    ACK 92b35aba224ad4440f3ea6c01c841596a6a3d6f4 - code review only
  24. MarcoFalke merged this on May 10, 2022
  25. MarcoFalke closed this on May 10, 2022
  26. w0xlt deleted the branch on May 10, 2022
  27. sidhujag referenced this in commit 4c36a8bce3 on May 10, 2022
  28. DrahtBot locked this on May 13, 2023


w0xlt jonatack MarcoFalke ajtowns


ajtowns mzumsande

Labels
UTXO Db and Indexes

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 19:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me