-
Notifications
You must be signed in to change notification settings - Fork 34
Perf/cpu thread default #37
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,97 @@ | |
| #include "ggml.h" | ||
|
|
||
| #include <cstdio> | ||
| #include <cstdint> | ||
| #include <thread> | ||
|
|
||
| #if defined(__APPLE__) | ||
| # include <sys/sysctl.h> | ||
| #elif defined(_WIN32) | ||
| # ifndef WIN32_LEAN_AND_MEAN | ||
| # define WIN32_LEAN_AND_MEAN | ||
| # endif | ||
| # include <windows.h> | ||
| # include <vector> | ||
| #elif defined(__linux__) | ||
| # include <cstdio> | ||
| # include <set> | ||
| # include <utility> | ||
| #endif | ||
|
|
||
| namespace s2 { | ||
|
|
||
| // Number of physical "performance" cores available for compute. | ||
| // | ||
| // ggml's CPU backend scales poorly when oversubscribed onto hyperthreads or, | ||
| // on Apple Silicon, onto the slow efficiency cores: every thread in a fork/join | ||
| // matmul waits on the slowest one, so the whole step runs at efficiency-core | ||
| // speed. Measured on an M4 Pro (10P + 4E), defaulting to all 14 logical cores | ||
| // was ~2.9x slower end-to-end than capping at the 10 performance cores. | ||
| // | ||
| // This returns a sensible default core count; callers fall back to it only when | ||
| // the user did not pass an explicit --threads value. | ||
| inline int32_t detect_compute_threads() { | ||
| #if defined(__APPLE__) | ||
| // Performance-core count (perflevel0). Falls back to all physical cores. | ||
| int32_t n = 0; | ||
| size_t sz = sizeof(n); | ||
| if (sysctlbyname("hw.perflevel0.physicalcpu", &n, &sz, nullptr, 0) == 0 && n > 0) { | ||
| return n; | ||
| } | ||
| n = 0; sz = sizeof(n); | ||
| if (sysctlbyname("hw.physicalcpu", &n, &sz, nullptr, 0) == 0 && n > 0) { | ||
| return n; | ||
| } | ||
| #elif defined(_WIN32) | ||
| // Count physical cores via processor-core relationships. | ||
| DWORD len = 0; | ||
| GetLogicalProcessorInformation(nullptr, &len); | ||
| if (len > 0) { | ||
| std::vector<SYSTEM_LOGICAL_PROCESSOR_INFORMATION> buf( | ||
| len / sizeof(SYSTEM_LOGICAL_PROCESSOR_INFORMATION)); | ||
| if (GetLogicalProcessorInformation(buf.data(), &len)) { | ||
| int32_t cores = 0; | ||
| for (const auto & info : buf) { | ||
| if (info.Relationship == RelationProcessorCore) cores++; | ||
| } | ||
| if (cores > 0) return cores; | ||
| } | ||
| } | ||
|
Comment on lines
+47
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚀 Performance & Scalability | 🟡 Minor 🧩 Analysis chain🌐 Web query:
💡 Result: No, the GetLogicalProcessorInformation function does not report processors across all processor groups on systems with more than 64 logical processors [1][2]. On such systems, GetLogicalProcessorInformation only retrieves information about the processors within the specific processor group to which the calling thread is currently assigned [1][2][3]. To retrieve information about logical processors across all processor groups in the system, you must use the GetLogicalProcessorInformationEx function instead [1][2][4]. GetLogicalProcessorInformationEx is designed to handle systems with more than 64 processors by providing information for every active processor group [5]. Citations:
🏁 Script executed: # Inspect the relevant code and nearby fallback logic in src/s2_ggml_utils.h
sed -n '1,140p' src/s2_ggml_utils.hRepository: rodrigomatta/s2.cpp Length of output: 4861 🏁 Script executed: sed -n '1,140p' src/s2_ggml_utils.hRepository: rodrigomatta/s2.cpp Length of output: 4861 Use 🤖 Prompt for AI Agents |
||
| #elif defined(__linux__) | ||
| // Count distinct (physical_id, core_id) pairs from /proc/cpuinfo to skip | ||
| // SMT siblings. | ||
| if (FILE * f = std::fopen("/proc/cpuinfo", "r")) { | ||
| std::set<std::pair<int, int>> cores; | ||
| int phys = -1, core = -1; | ||
| char line[256]; | ||
| while (std::fgets(line, sizeof(line), f)) { | ||
| int v; | ||
| if (std::sscanf(line, "physical id : %d", &v) == 1) phys = v; | ||
| else if (std::sscanf(line, "core id : %d", &v) == 1) core = v; | ||
| if (line[0] == '\n') { // blank line separates logical CPUs | ||
| if (phys >= 0 && core >= 0) cores.insert({phys, core}); | ||
| phys = core = -1; | ||
| } | ||
| } | ||
| if (phys >= 0 && core >= 0) cores.insert({phys, core}); | ||
| std::fclose(f); | ||
| if (!cores.empty()) return static_cast<int32_t>(cores.size()); | ||
| } | ||
| #endif | ||
| // Generic fallback: assume SMT and halve the logical-core count, but never | ||
| // go below 1. | ||
| const unsigned hw = std::thread::hardware_concurrency(); | ||
| if (hw == 0) return 4; | ||
| return static_cast<int32_t>(hw >= 2 ? hw / 2 : hw); | ||
| } | ||
|
|
||
| // Resolve a caller-supplied thread count: honor explicit positive values, | ||
| // otherwise pick a good default for this machine. | ||
| inline int32_t resolve_n_threads(int32_t n_threads) { | ||
| if (n_threads > 0) return n_threads; | ||
| return detect_compute_threads(); | ||
| } | ||
|
|
||
| inline ggml_tensor * repeat_checked(ggml_context * ctx, ggml_tensor * a, ggml_tensor * b, | ||
| const char * label = "repeat") { | ||
| if (!ggml_can_repeat(a, b)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||||||||||||||||||||||||||||||||||||||||
| #include "../include/s2_sampler.h" | ||||||||||||||||||||||||||||||||||||||||||
| #include <cmath> | ||||||||||||||||||||||||||||||||||||||||||
| #include <algorithm> | ||||||||||||||||||||||||||||||||||||||||||
| #include <limits> | ||||||||||||||||||||||||||||||||||||||||||
| #include <random> | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| namespace s2 { | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -22,50 +23,58 @@ static void apply_softmax(std::vector<float> & probs, float temp = 1.0f) { | |||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| static std::vector<float> softmax_from_sorted_logits(const std::vector<std::pair<float, int32_t>> & items) { | ||||||||||||||||||||||||||||||||||||||||||
| std::vector<float> probs(items.size(), 0.0f); | ||||||||||||||||||||||||||||||||||||||||||
| if (items.empty()) return probs; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const float max_val = items.front().first; | ||||||||||||||||||||||||||||||||||||||||||
| float sum = 0.0f; | ||||||||||||||||||||||||||||||||||||||||||
| for (size_t i = 0; i < items.size(); ++i) { | ||||||||||||||||||||||||||||||||||||||||||
| probs[i] = std::exp(items[i].first - max_val); | ||||||||||||||||||||||||||||||||||||||||||
| sum += probs[i]; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| if (sum > 0.0f) { | ||||||||||||||||||||||||||||||||||||||||||
| for (float & p : probs) p /= sum; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| return probs; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| int32_t sample_token(const float * logits, int32_t vocab_size, const SamplerParams & params) { | ||||||||||||||||||||||||||||||||||||||||||
| if (vocab_size <= 0) return 0; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const int32_t k = params.top_k > 0 ? std::min(params.top_k, vocab_size) : vocab_size; | ||||||||||||||||||||||||||||||||||||||||||
| const float top_p = std::clamp(params.top_p, 0.0f, 1.0f); | ||||||||||||||||||||||||||||||||||||||||||
| const float NEG_INF = -std::numeric_limits<float>::infinity(); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Single pass: gather finite candidates and the global max. Masked logits | ||||||||||||||||||||||||||||||||||||||||||
| // (set to -inf by the caller, e.g. the semantic mask covering ~95% of the | ||||||||||||||||||||||||||||||||||||||||||
| // vocab) contribute exp(-inf)=0 to the softmax and can never be selected, | ||||||||||||||||||||||||||||||||||||||||||
| // so skipping them is exact and avoids building/sorting the full vocab. | ||||||||||||||||||||||||||||||||||||||||||
| std::vector<std::pair<float, int32_t>> items; | ||||||||||||||||||||||||||||||||||||||||||
| items.reserve(vocab_size); | ||||||||||||||||||||||||||||||||||||||||||
| items.reserve(static_cast<size_t>(std::min(vocab_size, 8192))); | ||||||||||||||||||||||||||||||||||||||||||
| float max_val = NEG_INF; | ||||||||||||||||||||||||||||||||||||||||||
| for (int32_t i = 0; i < vocab_size; ++i) { | ||||||||||||||||||||||||||||||||||||||||||
| items.push_back({logits[i], i}); | ||||||||||||||||||||||||||||||||||||||||||
| const float v = logits[i]; | ||||||||||||||||||||||||||||||||||||||||||
| if (v == NEG_INF) continue; | ||||||||||||||||||||||||||||||||||||||||||
| items.push_back({v, i}); | ||||||||||||||||||||||||||||||||||||||||||
| if (v > max_val) max_val = v; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
40
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check includes and NEG_INF definition in src/s2_sampler.cpp
cat -n src/s2_sampler.cpp | head -100Repository: rodrigomatta/s2.cpp Length of output: 4539 🏁 Script executed: python3 - <<'PY'
import math
# Verify isfinite behavior for edge cases
test_values = [
(float('-inf'), False, "NEG_INF"),
(float('inf'), False, "+inf"),
(float('nan'), False, "NaN"),
(0.0, True, "zero"),
(-10.0, True, "negative"),
(10.0, True, "positive"),
]
print("Testing isfinite behavior:")
for val, expected_finite, desc in test_values:
is_finite = math.isfinite(val)
status = "✓" if is_finite == expected_finite else "✗"
print(f"{status} {desc}: value={val}, is_finite={is_finite}, expected={expected_finite}")
print("\nAll edge cases handled correctly by isfinite:")
print("- NaN: filtered out (not finite)")
print("- +inf: filtered out (not finite)")
print("- -inf: filtered out (not finite)")
print("- Finite values: kept (finite)")
PYRepository: rodrigomatta/s2.cpp Length of output: 677 Filter all non-finite logits, not only exact The current check Proposed fix for (int32_t i = 0; i < vocab_size; ++i) {
const float v = logits[i];
- if (v == NEG_INF) continue;
+ if (!std::isfinite(v)) continue;
items.push_back({v, i});
if (v > max_val) max_val = v;
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
| if (items.empty()) return 0; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Full partition function over the finite candidates. This matches the | ||||||||||||||||||||||||||||||||||||||||||
| // original code's denominator (the -inf terms it summed were exactly 0), | ||||||||||||||||||||||||||||||||||||||||||
| // so the top_p cumulative mass below is computed identically. | ||||||||||||||||||||||||||||||||||||||||||
| float Z = 0.0f; | ||||||||||||||||||||||||||||||||||||||||||
| for (const auto & it : items) Z += std::exp(it.first - max_val); | ||||||||||||||||||||||||||||||||||||||||||
| if (!(Z > 0.0f)) Z = 1.0f; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Order by logit descending, breaking ties by index for determinism. | ||||||||||||||||||||||||||||||||||||||||||
| auto cmp = [](const std::pair<float, int32_t> & a, const std::pair<float, int32_t> & b) { | ||||||||||||||||||||||||||||||||||||||||||
| if (a.first != b.first) return a.first > b.first; | ||||||||||||||||||||||||||||||||||||||||||
| return a.second < b.second; | ||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||
| const int32_t kk = std::min<int32_t>(k, static_cast<int32_t>(items.size())); | ||||||||||||||||||||||||||||||||||||||||||
| if (kk < static_cast<int32_t>(items.size())) { | ||||||||||||||||||||||||||||||||||||||||||
| // top_k is small (default 30); only the top kk elements are ever kept, | ||||||||||||||||||||||||||||||||||||||||||
| // so a partial sort (O(n log kk)) replaces a full sort (O(n log n)). | ||||||||||||||||||||||||||||||||||||||||||
| std::partial_sort(items.begin(), items.begin() + kk, items.end(), cmp); | ||||||||||||||||||||||||||||||||||||||||||
| items.resize(kk); | ||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||
| std::sort(items.begin(), items.end(), cmp); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| std::sort(items.begin(), items.end(), [](const auto & a, const auto & b) { | ||||||||||||||||||||||||||||||||||||||||||
| return a.first > b.first; | ||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| const int32_t k = params.top_k > 0 ? std::min(params.top_k, vocab_size) : vocab_size; | ||||||||||||||||||||||||||||||||||||||||||
| const float top_p = std::clamp(params.top_p, 0.0f, 1.0f); | ||||||||||||||||||||||||||||||||||||||||||
| const std::vector<float> sorted_probs = softmax_from_sorted_logits(items); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Keep the prefix up to top_k and the top_p cumulative-probability cutoff. | ||||||||||||||||||||||||||||||||||||||||||
| std::vector<std::pair<float, int32_t>> filtered; | ||||||||||||||||||||||||||||||||||||||||||
| filtered.reserve(k); | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| filtered.reserve(items.size()); | ||||||||||||||||||||||||||||||||||||||||||
| float cumsum = 0.0f; | ||||||||||||||||||||||||||||||||||||||||||
| for (int32_t i = 0; i < (int32_t)items.size(); ++i) { | ||||||||||||||||||||||||||||||||||||||||||
| cumsum += sorted_probs[i]; | ||||||||||||||||||||||||||||||||||||||||||
| const bool remove_for_top_k = (i >= k); | ||||||||||||||||||||||||||||||||||||||||||
| const bool remove_for_top_p = (i > 0 && cumsum > top_p); | ||||||||||||||||||||||||||||||||||||||||||
| if (remove_for_top_k || remove_for_top_p) { | ||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||
| cumsum += std::exp(items[i].first - max_val) / Z; | ||||||||||||||||||||||||||||||||||||||||||
| if (i > 0 && cumsum > top_p) { | ||||||||||||||||||||||||||||||||||||||||||
| break; // cumsum is monotonic, so every later item also exceeds top_p | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| filtered.push_back(items[i]); | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
73
to
79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Include the token that crosses the The current loop breaks before adding the crossing token, so Proposed fix float cumsum = 0.0f;
for (int32_t i = 0; i < (int32_t)items.size(); ++i) {
cumsum += std::exp(items[i].first - max_val) / Z;
- if (i > 0 && cumsum > top_p) {
- break; // cumsum is monotonic, so every later item also exceeds top_p
- }
filtered.push_back(items[i]);
+ if (top_p < 1.0f && cumsum >= top_p) {
+ break; // cumsum is monotonic, so every later item also exceeds top_p
+ }
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Handle multi-config generators here.
This default only works with single-config generators because
CMAKE_BUILD_TYPEis empty on Visual Studio/Xcode/Ninja Multi-Config. On those generators, Release/RelWithDebInfo/MinSizeRel builds will still inheritGGML_LTO=OFF, so the new default never takes effect. Consider also checkingCMAKE_CONFIGURATION_TYPES(or otherwise defaultingGGML_LTOfor multi-config generators when the cache entry is unset).🤖 Prompt for AI Agents