pingcap / tiflash

The analytical engine for TiDB and TiDB Cloud. Try free: https://tidbcloud.com/free-trial
https://docs.pingcap.com/tidb/stable/tiflash-overview
Apache License 2.0
941 stars 409 forks source link

Reduce paddings between fields in stackable classes #8758

Open CalvinNeo opened 7 months ago

CalvinNeo commented 7 months ago

Enhancement

CalvinNeo commented 7 months ago

https://stackoverflow.com/questions/32007188/detect-if-struct-has-padding This is the way to check if a class has padding.

JaySon-Huang commented 7 months ago

A class has padding does not mean it can become a more memory-compact layout.

image

CalvinNeo commented 7 months ago

@JaySon-Huang Yes you are right, reducing the size of Region by reorg fields is hard.

Size of KVStore can be improved, however, not necessary. The size changed from 1280 to 1216

...
    RegionManager region_manager;

    mutable std::mutex task_mutex;
    mutable std::mutex bg_gc_region_data_mutex;

    // raft_cmd_res stores the result of applying raft cmd. It must be protected by task_mutex.
    std::unique_ptr<RaftCommandResult> raft_cmd_res;
    std::unique_ptr<RegionPersister> region_persister;
    LoggerPtr log;
    ReadIndexWorkerManager * read_index_worker_manager{nullptr};
    const TiFlashRaftProxyHelper * proxy_helper{nullptr};
    std::atomic<UInt64> region_compact_log_min_rows;
    std::atomic<UInt64> region_compact_log_min_bytes;
    std::atomic<UInt64> region_compact_log_gap;
    std::atomic<int64_t> ongoing_prehandle_task_count{0};
    std::atomic<UInt64> region_eager_gc_log_gap;
    std::atomic_int64_t read_index_event_flag{0};
    const bool eager_raft_log_gc_enabled;
    std::atomic<Timepoint> last_gc_time = Timepoint::min();

    std::list<RegionDataReadInfoList> bg_gc_region_data;

    PreHandlingTrace prehandling_trace;
    StoreMeta store;
    RaftLogEagerGcTasks raft_log_gc_hints;
    ProxyConfigSummary proxy_config_summary;
CalvinNeo commented 7 months ago

Alright, the reorg-structure thing is not always make sense, see https://stackoverflow.com/questions/14671253/is-there-a-gcc-keyword-to-allow-structure-reordering/28780286. So arguemnts:

  1. Hard to always work correctly
  2. May not work well with LTO
  3. It is better introduce a [[reorder]] attribute to explicit allows the compiler to reorder layout to have a better layout.