ronantakizawa/codereview-bench huggingface dataset:
Repository: vespa-engine/vespa
File: searchlib/src/vespa/searchlib/tensor/serialized_fast_value_attribute.cpp (C++)

Code before:
bool
SerializedFastValueAttribute::supports_get_serialized_tensor_ref() const
{
    return true;
}

vespalib::eval::TypedCells
SerializedFastValueAttribute::get_vector(uint32_t docid, uint32_t subspace) const noexcept
{
    EntryRef ref = acquire_entry_ref(docid);
    auto vectors = _tensorBufferStore.get_vectors(ref);
    return (subspace < vectors.subspaces()) ? vectors.cells(subspace) : _tensorBufferStore.get_empty_subspace();
}

VectorBundle
SerializedFastValueAttribute::get_vectors(uint32_t docid) const noexcept
{
    EntryRef ref = acquire_entry_ref(docid);
    return _tensorBufferStore.get_vectors(ref);
}

void
SerializedFastValueAttribute::prefetch_docid(uint32_t docid) const noexcept
{
    const auto* storage_start = std::addressof(_refVector.acquire_elem_ref(0));
    __builtin_prefetch(storage_start + docid);
}

void
SerializedFastValueAttribute::prefetch_vector(uint32_t docid) const noexcept
{
    const auto ref = acquire_entry_ref(docid);
    _tensorBufferStore.prefetch_vectors(ref);
}

}


Review comment (suggestion):
I think this is a bit too leaky with regards to `RcuVector`'s implementation. Consider adding `prefetch_elem_ref` to `RcuVectorBase` to hide the details

Code after:
bool
SerializedFastValueAttribute::supports_get_serialized_tensor_ref() const
{
    return true;
}

vespalib::eval::TypedCells
SerializedFastValueAttribute::get_vector(uint32_t docid, uint32_t subspace) const noexcept
{
    EntryRef ref = acquire_entry_ref(docid);
    auto vectors = _tensorBufferStore.get_vectors(ref);
    return (subspace < vectors.subspaces()) ? vectors.cells(subspace) : _tensorBufferStore.get_empty_subspace();
}

VectorBundle
SerializedFastValueAttribute::get_vectors(uint32_t docid) const noexcept
{
    EntryRef ref = acquire_entry_ref(docid);
    return _tensorBufferStore.get_vectors(ref);
}

void
SerializedFastValueAttribute::prefetch_vector(uint32_t docid) const noexcept
{
    const auto ref = acquire_entry_ref(docid);
    _tensorBufferStore.prefetch_vectors(ref);
}

}
Repository: vespa-engine/vespa
File: searchlib/src/vespa/searchlib/tensor/tensor_buffer_store.h (C/C++)

Code before:
    vespalib::eval::TypedCells get_empty_subspace() const noexcept {
        return _ops.get_empty_subspace();
    }
    VectorBundle get_vectors(EntryRef ref) const noexcept {
        if (!ref.valid()) {
            return {};
        }
        auto buf = _array_store.get(ref);
        return _ops.get_vectors(buf);
    }
    SerializedTensorRef get_serialized_tensor_ref(EntryRef ref) const noexcept {
        if (!ref.valid()) {
            return {};
        }
        auto buf = _array_store.get(ref);
        return _ops.get_serialized_tensor_ref(buf);
    }
    void prefetch_vectors(EntryRef ref) const noexcept {
        const auto buf = _array_store.get(ref);

        for (std::size_t i = 0; i < buf.size(); i += 64) {
            __builtin_prefetch(buf.data() + i);
        }
    }

    // Used by unit test
    static constexpr uint32_t get_offset_bits() noexcept { return RefType::offset_bits; }
};

}


Review comment (performance):
I have concerns about this inherently prefetching _everything_ across all subspaces, which can pollute the caches with stuff that will not be used (especially L1d)... Conceptually, sparse subspaces are unbounded, so this has the potential of blowing _all_ cache hierarchies out of the water.

The first cache line of `buf` holds the subspace header, so maybe that should be prefetched first, then prefetch the actual subspace afterwards (`get_vector(docid, subspace)`).

However, I think any vector prefetching functionality should first be implemented for explicitly _dense_ tensors, as we always want to look at the entire buffer for those.

Code after:
    vespalib::eval::TypedCells get_empty_subspace() const noexcept {
        return _ops.get_empty_subspace();
    }
    VectorBundle get_vectors(EntryRef ref) const noexcept {
        if (!ref.valid()) {
            return {};
        }
        auto buf = _array_store.get(ref);
        return _ops.get_vectors(buf);
    }
    SerializedTensorRef get_serialized_tensor_ref(EntryRef ref) const noexcept {
        if (!ref.valid()) {
            return {};
        }
        auto buf = _array_store.get(ref);
        return _ops.get_serialized_tensor_ref(buf);
    }
    void prefetch_vectors(EntryRef ref) const noexcept {
        if (!ref.valid()) [[unlikely]] {
            return;
        }

        const auto buf = _array_store.get(ref);
        for (size_t i = 0; i < buf.size(); i += 64) {
            __builtin_prefetch(buf.data() + i);
        }
    }

    // Used by unit test
    static constexpr uint32_t get_offset_bits() noexcept { return RefType::offset_bits; }
};

}
Repository: vespa-engine/vespa
File: searchlib/src/vespa/searchlib/tensor/doc_vector_access.h (C/C++)

Code before:
// Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.

#pragma once

#include <vespa/eval/eval/typed_cells.h>
#include <cstdint>

namespace search::tensor {

class VectorBundle;

/**
 * Interface that provides access to the vector that is associated with the the given document id.
 *
 * All vectors should be the same size and either of type float or double.
 */
class DocVectorAccess {
public:
    virtual ~DocVectorAccess() = default;
    virtual vespalib::eval::TypedCells get_vector(uint32_t docid, uint32_t subspace) const noexcept = 0;
    virtual VectorBundle get_vectors(uint32_t docid) const noexcept = 0;

    virtual void prefetch_docid(uint32_t) const noexcept {}
    virtual void prefetch_vector(uint32_t) const noexcept {}
};

}


Review comment (suggestion):
Consider adding names to these parameters since their semantics can't necessarily be inferred from the generic `uint32_t` type alone (will need a `[[maybe_unused]]` or a `(void)param_name`)

Code after:
// Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.

#pragma once

#include <vespa/eval/eval/typed_cells.h>
#include <cstdint>

namespace search::tensor {

class VectorBundle;

/**
 * Interface that provides access to the vector that is associated with the the given document id.
 *
 * All vectors should be the same size and either of type float or double.
 */
class DocVectorAccess {
public:
    virtual ~DocVectorAccess() = default;
    virtual vespalib::eval::TypedCells get_vector(uint32_t docid, uint32_t subspace) const noexcept = 0;
    virtual VectorBundle get_vectors(uint32_t docid) const noexcept = 0;

    /**
     * Try to prefetch into memory data needed to resolve docid into corresponding tensor.
     *
     * In some implementations in order to resolve docid into corresponding tensor we have to go
     * through a level of indirection, which might cause memory-cache misses on its own.
     * In such implementations one could implement this method to prefetch this indirection first,
     * and the calling code would try its best to give this prefetch enough time to bring the
     * data needed in before invoking `prefetch_vector`.
     */
    virtual void prefetch_docid(uint32_t docid) const noexcept {
        (void)docid;
    }

    /**
     * Try to prefetch tensor's data into memory.
     */
    virtual void prefetch_vector(uint32_t docid) const noexcept {
        (void)docid;
    }
};

}
Repository: vespa-engine/vespa
File: vespajlib/src/main/java/com/yahoo/data/access/Inspector.java (Java)

Code before:
    Inspector field(String name);

    /**
     * Convert an array to an iterable list.  Other types will just
     * return an empty list.
     */
    Iterable<Inspector> entries();

    /**
     * Convert an object to an iterable list of (name, value) pairs.
     * Other types will just return an empty list.
     */
    Iterable<Map.Entry<String,Inspector>> fields();

    /**
     * Default implementation of emitting an inspector. Defaults to converting
     * strings to UTF-8.
     */
    @Override
    default void emit(DataSink sink) {
        switch (type()) {
            case EMPTY -> sink.emptyValue();
            case BOOL -> sink.booleanValue(asBool());
            case LONG -> sink.longValue(asLong());
            case DOUBLE -> sink.doubleValue(asDouble());
            case STRING -> sink.stringValue(asUtf8());
            case DATA -> sink.dataValue(asData());
            case ARRAY -> {
                sink.startArray();
                for (var entry : entries()) {
                    entry.emit(sink);
                }
                sink.endArray();
            }
            case OBJECT -> {
                sink.startObject();
                for (var field : fields()) {
                    sink.fieldName(field.getKey());
                    field.getValue().emit(sink);
                }
                sink.endObject();
            }
        }
    }
}


Review comment (suggestion):
the default string type should be utf16, all subclasses having utf8 as an option should override this method

Code after:
    Inspector field(String name);

    /**
     * Convert an array to an iterable list.  Other types will just
     * return an empty list.
     */
    Iterable<Inspector> entries();

    /**
     * Convert an object to an iterable list of (name, value) pairs.
     * Other types will just return an empty list.
     */
    Iterable<Map.Entry<String,Inspector>> fields();

    /**
     * Default implementation of emitting an inspector. Defaults to converting
     * strings to UTF-16.
     */
    @Override
    default void emit(DataSink sink) {
        switch (type()) {
            case EMPTY -> sink.emptyValue();
            case BOOL -> sink.booleanValue(asBool());
            case LONG -> sink.longValue(asLong());
            case DOUBLE -> sink.doubleValue(asDouble());
            case STRING -> sink.stringValue(asString());
            case DATA -> sink.dataValue(asData());
            case ARRAY -> {
                sink.startArray();
                for (var entry : entries()) {
                    entry.emit(sink);
                }
                sink.endArray();
            }
            case OBJECT -> {
                sink.startObject();
                for (var field : fields()) {
                    sink.fieldName(field.getKey());
                    field.getValue().emit(sink);
                }
                sink.endObject();
            }
        }
    }
}
Repository: vespa-engine/vespa
File: searchcore/src/vespa/searchcore/proton/flushengine/reserved_disk_space_calculator.cpp (C++)

Code before:
    : _concurrent(concurrent),
      _candidates(),
      _reserved_grow(0)
{
}

ReservedDiskSpaceCalculator::~ReservedDiskSpaceCalculator() = default;

void
ReservedDiskSpaceCalculator::track_disk_gain(const IFlushTarget::DiskGain &gain)
{
    if (gain.getAfter() > gain.getBefore()) {
        _reserved_grow += gain.getAfter() - gain.getBefore();
    }
    _candidates.emplace_back(gain.getAfter());
}

uint64_t
ReservedDiskSpaceCalculator::get_reserved_disk()
{
    uint64_t reserved = _reserved_grow;
    if (_concurrent < _candidates.size()) {
        std::nth_element(_candidates.begin(), _candidates.begin() + _concurrent, _candidates.end());
        _candidates.resize(_concurrent);
    }
    for (auto &candidate : _candidates) {
        reserved += candidate.reserved();
    }
    return reserved;
}

}


Review comment (suggestion):
I think it could be useful with a very brief comment explaining that this partitions the candidate set so that only the biggest candidates are retained, since you otherwise have to know that `operator<` is overloaded for this value type (i.e. at first glance it might seem like the _opposite_ ordering is the case).

Code after:
    : _concurrent(concurrent),
      _candidates(),
      _reserved_grow(0)
{
}

ReservedDiskSpaceCalculator::~ReservedDiskSpaceCalculator() = default;

void
ReservedDiskSpaceCalculator::track_disk_gain(const IFlushTarget::DiskGain &gain)
{
    if (gain.getAfter() > gain.getBefore()) {
        _reserved_grow += gain.getAfter() - gain.getBefore();
    }
    _candidates.emplace_back(gain.getAfter());
}

uint64_t
ReservedDiskSpaceCalculator::get_reserved_disk()
{
    if (_concurrent < _candidates.size()) {
        /*
         * Retain the _concurrent biggest candidates. They are later used to calculate reserved
         * disk space for flush.
         */
        std::nth_element(_candidates.begin(), _candidates.begin() + _concurrent, _candidates.end());
        _candidates.resize(_concurrent);
    }
    uint64_t reserved_flush = 0;
    for (auto &candidate : _candidates) {
        reserved_flush += candidate.reserved();
    }
    return _reserved_grow + reserved_flush;
}

}
Repository: vespa-engine/vespa
File: searchlib/src/vespa/searchlib/docstore/data_store_storage_stats.h (C/C++)

Code before:
    uint64_t _diskBloat;
    double   _maxBucketSpread;
    uint64_t _lastSerialNum;
    uint64_t _lastFlushedSerialNum;
    uint32_t _docIdLimit;
public:
    DataStoreStorageStats(uint64_t diskUsage_in, uint64_t size_on_disk_in, uint64_t diskBloat_in,
                          double maxBucketSpread_in, uint64_t lastSerialNum_in, uint64_t lastFlushedSerialNum_in,
                          uint32_t docIdLimit_in) noexcept
        : _diskUsage(diskUsage_in),
          _size_on_disk(size_on_disk_in),
          _diskBloat(diskBloat_in),
          _maxBucketSpread(maxBucketSpread_in),
          _lastSerialNum(lastSerialNum_in),
          _lastFlushedSerialNum(lastFlushedSerialNum_in),
          _docIdLimit(docIdLimit_in)
    { }
    uint64_t diskUsage() const            { return _diskUsage; }
    uint64_t size_on_disk() const noexcept { return _size_on_disk; }
    uint64_t diskBloat() const            { return _diskBloat; }
    double   maxBucketSpread() const      { return _maxBucketSpread; }
    uint64_t lastSerialNum() const        { return _lastSerialNum; }
    uint64_t lastFlushedSerialNum() const { return _lastFlushedSerialNum; }
    uint32_t docIdLimit() const           { return _docIdLimit; }
};

} // namespace search


Review comment (suggestion):
For posterity I think it would be useful with a comment describing the difference between `diskUsage` and `size_on_disk`

Code after:
    uint64_t _diskBloat;
    double   _maxBucketSpread;
    uint64_t _lastSerialNum;
    uint64_t _lastFlushedSerialNum;
    uint32_t _docIdLimit;
public:
    DataStoreStorageStats(uint64_t diskUsage_in, uint64_t size_on_disk_in, uint64_t diskBloat_in,
                          double maxBucketSpread_in, uint64_t lastSerialNum_in, uint64_t lastFlushedSerialNum_in,
                          uint32_t docIdLimit_in) noexcept
        : _diskUsage(diskUsage_in),
          _size_on_disk(size_on_disk_in),
          _diskBloat(diskBloat_in),
          _maxBucketSpread(maxBucketSpread_in),
          _lastSerialNum(lastSerialNum_in),
          _lastFlushedSerialNum(lastFlushedSerialNum_in),
          _docIdLimit(docIdLimit_in)
    { }
    /*
     * Sum of file sizes.
     */
    uint64_t diskUsage() const            { return _diskUsage; }
    /*
     * Sum of file sizes each aligned up to assumed file system block size and a placeholder directory size
     * (when not part of DataStoreFileChunkStats).
     */
    uint64_t size_on_disk() const noexcept { return _size_on_disk; }
    uint64_t diskBloat() const            { return _diskBloat; }
    double   maxBucketSpread() const      { return _maxBucketSpread; }
    uint64_t lastSerialNum() const        { return _lastSerialNum; }
    uint64_t lastFlushedSerialNum() const { return _lastFlushedSerialNum; }
    uint32_t docIdLimit() const           { return _docIdLimit; }
};

} // namespace search
Repository: vespa-engine/vespa
File: searchlib/src/vespa/searchlib/expression/attributenode.cpp (C++)

Code before:
{
    const IAttributeVector * attribute(_scratchResult ? _scratchResult->getAttribute() : nullptr);
    if (attribute == nullptr) {
        if (_useEnumOptimization) {
            attribute = attrCtx.getAttributeStableEnum(_attributeName);
        } else {
            attribute = attrCtx.getAttribute(_attributeName);
        }
        if (attribute == nullptr) {
            throw std::runtime_error(make_string("Failed locating attribute vector '%s'", _attributeName.c_str()));
        }
        _hasMultiValue = attribute->hasMultiValue();
        _scratchResult = createResult(attribute);
    }
}

void
AttributeNode::cleanup()
{
    _scratchResult.reset();
}

Serializer &
AttributeNode::onSerialize(Serializer & os) const
{
    FunctionNode::onSerialize(os);
    return os << _attributeName;
}

Deserializer &
AttributeNode::onDeserialize(Deserializer & is)
{
    FunctionNode::onDeserialize(is);
    return is >> _attributeName;
}

void
AttributeNode::visitMembers(vespalib::ObjectVisitor &visitor) const
{
    visit(visitor, "attributeName", _attributeName);
}

}

// this function was added by ../../forcelink.sh
void forcelink_file_searchlib_expression_attributenode() {}


Review comment (suggestion):
Consider deduping these `handle()` function bodies

Code after:
            _vector[i] = values[i];
        }
    }
}

void
AttributeNode::StringHandler::handle(const AttributeResult & r)
{
    if (_read_view != nullptr) {
        auto values = _read_view->get_values(r.getDocId());
        _vector.resize(values.size());
        for(size_t i(0); i < values.size(); i++) {
            _vector[i] = values[i];
        }
    }
}

void
AttributeNode::EnumHandler::handle(const AttributeResult & r)
{
    if (_read_view != nullptr) {
        auto values = _read_view->get_values(r.getDocId());
        _vector.resize(values.size());
        for(size_t i(0); i < values.size(); i++) {
            _vector[i] = values[i].load_relaxed().ref();
        }
    }
}

void
AttributeNode::setDocId(DocId docId) {
    if (_scratchResult->getDocId() != docId) {
        _scratchResult->setDocId(docId);
        _needExecute = true;
    }
}

bool
AttributeNode::onExecute() const
{
    if (_handler) {
        if (_needExecute) {
            _handler->handle(*_scratchResult);
            _needExecute = false;
        }
        if (_currentIndex != nullptr) {
            assert(_keepAliveForIndexLookups);
            assert(_hasMultiValue);
            size_t idx = _currentIndex->get();
            if (idx < _keepAliveForIndexLookups->size()) [[likely]] {
                updateResult().set(_keepAliveForIndexLookups->get(idx));
Repository: vespa-engine/vespa
File: container-search/src/test/java/com/yahoo/prelude/semantics/test/WeakAndTestCase.java (Java)

Code before:
import com.yahoo.language.simple.SimpleLinguistics;
import com.yahoo.prelude.IndexFacts;
import com.yahoo.prelude.IndexModel;
import com.yahoo.search.Query;
import com.yahoo.search.Result;
import com.yahoo.search.searchchain.Execution;
import com.yahoo.search.test.QueryTestCase;
import com.yahoo.search.yql.MinimalQueryInserter;
import org.junit.jupiter.api.Test;

import java.util.List;
import java.util.Map;

/**
 * @author bratseth
 */
public class WeakAndTestCase {

    @Test
    public void testWeakAnd() {
        var tester = new RuleBaseTester("weakAnd.sr", null);
        var query = new Query("?yql=" + QueryTestCase.httpEncode("select * from webpages where (userQuery() and  (hostname contains 'www.legifrance.gouv.fr'))") + "&query=tva");
        search(new SimpleLinguistics(), new IndexModel(Map.of(), List.of()), query);
        System.out.println("Before: " + query.getModel().getQueryTree().toString());
        tester.assertSemantics("AND (WEAKAND tva taxe sur la valeur ajoutée tva) hostname:'www legifrance gouv fr'", query);
        System.out.println("After:  " + query.getModel().getQueryTree().toString());
    }

    private Result search(Linguistics linguistics, IndexModel indexModel, Query query) {
        return new Execution(new Chain<>(new MinimalQueryInserter(linguistics) /*, new StemmingSearcher(linguistics)*/),
                             Execution.Context.createContextStub(new IndexFacts(indexModel), linguistics)).search(query);

    }

}


Review comment (bug):
The `System.out.println` calls on lines 29 and 31 look like debug leftovers. Remove?

Code after:
import com.yahoo.language.simple.SimpleLinguistics;
import com.yahoo.prelude.IndexFacts;
import com.yahoo.prelude.IndexModel;
import com.yahoo.search.Query;
import com.yahoo.search.Result;
import com.yahoo.search.searchchain.Execution;
import com.yahoo.search.test.QueryTestCase;
import com.yahoo.search.yql.MinimalQueryInserter;
import org.junit.jupiter.api.Test;

import java.util.List;
import java.util.Map;

/**
 * @author bratseth
 */
public class WeakAndTestCase {

    @Test
    public void testWeakAnd() {
        var tester = new RuleBaseTester("weakAnd.sr", null);
        var query = new Query("?yql=" + QueryTestCase.httpEncode("select * from webpages where (userQuery() and  (hostname contains 'www.legifrance.gouv.fr'))") + "&query=tva");
        search(new SimpleLinguistics(), new IndexModel(Map.of(), List.of()), query);
        tester.assertSemantics("AND (WEAKAND tva taxe sur la valeur ajoutée tva) hostname:'www legifrance gouv fr'", query);
    }

    private Result search(Linguistics linguistics, IndexModel indexModel, Query query) {
        return new Execution(new Chain<>(new MinimalQueryInserter(linguistics) /*, new StemmingSearcher(linguistics)*/),
                             Execution.Context.createContextStub(new IndexFacts(indexModel), linguistics)).search(query);

    }

}
Repository: vitejs/vite
File: packages/vite/src/node/plugins/license.ts (TypeScript)

Code before:
import fs from 'node:fs'
import path from 'node:path'
import type { Plugin } from '../plugin'
import { isInNodeModules, sortObjectKeys } from '../utils'
import type { PackageCache } from '../packages'
import { findNearestMainPackageData } from '../packages'

export interface LicenseEntry {
  /**
   * Package name
   */
  name: string
  /**
   * Package version
   */
  version: string
  /**
   * SPDX license identifier (from package.json "license" field)
   */
  identifier?: string
  /**
   * License file text
   */
  text?: string
}

// https://github.com/npm/npm-packlist/blob/53b2a4f42b7fef0f63e8f26a3ea4692e23a58fed/lib/index.js#L284-L286
const licenseFiles = [/^license/i, /^licence/i, /^copying/i]

export interface ManifestChunk {
  src?: string
  file: string
  css?: string[]
  assets?: string[]
  isEntry?: boolean
  name?: string
  isDynamicEntry?: boolean
  imports?: string[]
  dynamicImports?: string[]
}

export function licensePlugin(): Plugin {
  return {
    name: 'vite:manifest',

    async generateBundle(_, bundle) {
      const licenseOption = this.environment.config.build.license
      if (licenseOption === false) return


Review comment (security):
Wouldn't it be better to add the author and repository url?

https://github.com/vitejs/vite/blob/3400a5e258a597499c0f0808c8fca4d92eeabc17/packages/create-vite/LICENSE#L154-L155

Code after:
import fs from 'node:fs'
import path from 'node:path'
import type { Plugin } from '../plugin'
import { isInNodeModules, sortObjectKeys } from '../utils'
import type { PackageCache } from '../packages'
import { findNearestMainPackageData } from '../packages'

export interface LicenseEntry {
  /**
   * Package name
   */
  name: string
  /**
   * Package version
   */
  version: string
  /**
   * SPDX license identifier (from package.json "license" field)
   */
  identifier?: string
  /**
   * License file text
   */
  text?: string
}

export interface LicenseOptions {
  /**
   * The output file name of the license file relative to the output directory.
   * Specify a path that ends with `.json` to output the raw JSON metadata.
   *
   * @default '.vite/license.md'
   */
  fileName: string
}

const licenseConfigDefaults = Object.freeze({
  fileName: '.vite/license.md',
} satisfies LicenseOptions)

// https://github.com/npm/npm-packlist/blob/53b2a4f42b7fef0f63e8f26a3ea4692e23a58fed/lib/index.js#L284-L286
const licenseFiles = [/^license/i, /^licence/i, /^copying/i]

export function licensePlugin(): Plugin {
  return {
    name: 'vite:license',

    async generateBundle(_, bundle) {
      const licenseOption = this.environment.config.build.license