diff options
44 files changed, 538 insertions, 260 deletions
diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/Cloud.java b/config-provisioning/src/main/java/com/yahoo/config/provision/Cloud.java index a73c826c6b9..35e3a2bf5e2 100644 --- a/config-provisioning/src/main/java/com/yahoo/config/provision/Cloud.java +++ b/config-provisioning/src/main/java/com/yahoo/config/provision/Cloud.java @@ -34,6 +34,7 @@ public class Cloud { } /** Returns whether upgrading OS on hosts in this requires the host to be reprovisioned */ + // TODO(mpolden): Unused, remove this public boolean reprovisionToUpgradeOs() { return reprovisionToUpgradeOs; } diff --git a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java index fb88ce7886a..bf9033fa7c6 100644 --- a/flags/src/main/java/com/yahoo/vespa/flags/Flags.java +++ b/flags/src/main/java/com/yahoo/vespa/flags/Flags.java @@ -460,6 +460,13 @@ public class Flags { "Set mode for CSRF filter ('disabled', 'log_only', 'enabled')", "Takes effect on controller restart/redeployment"); + public static final UnboundBooleanFlag SOFT_REBUILD = defineFeatureFlag( + "soft-rebuild", false, + List.of("mpolden"), "2022-09-27", "2022-12-01", + "Whether soft rebuild can be used to rebuild hosts with remote disk", + "Takes effect on next run of OsUpgradeActivator" + ); + public static final UnboundListFlag<String> CSRF_USERS = defineListFlag( "csrf-users", List.of(), String.class, List.of("bjorncs", "tokle"), "2022-09-22", "2023-06-01", diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java index cde7f300f2b..9cba823500b 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/Node.java @@ -258,6 +258,10 @@ public final class Node implements Nodelike { if (wantToRetire == status.wantToRetire() && wantToDeprovision == status.wantToDeprovision() && wantToRebuild == status.wantToRebuild()) return this; + if (wantToRebuild && !wantToRetire && resources().storageType() != NodeResources.StorageType.remote) { + throw new IllegalArgumentException("Cannot rebuild " + this + " without retiring because storage is " + + resources().storageType()); + } Node node = this.with(status.withWantToRetire(wantToRetire, wantToDeprovision, wantToRebuild)); if (wantToRetire) node = node.with(history.with(new History.Event(History.Event.Type.wantToRetire, agent, at))); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java index dd4d5aa213f..58535b54a1b 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/NodeList.java @@ -50,8 +50,13 @@ public class NodeList extends AbstractFilteringList<Node, NodeList> { } /** Returns the subset of nodes that are being rebuilt */ - public NodeList rebuilding() { - return matching(node -> node.status().wantToRetire() && node.status().wantToRebuild()); + public NodeList rebuilding(boolean soft) { + return matching(node -> { + if (soft) { + return !node.status().wantToRetire() && node.status().wantToRebuild(); + } + return node.status().wantToRetire() && node.status().wantToRebuild(); + }); } /** Returns the subset of nodes which are removable */ @@ -67,6 +72,11 @@ public class NodeList extends AbstractFilteringList<Node, NodeList> { /** Returns the subset of nodes having exactly the given resources */ public NodeList resources(NodeResources resources) { return matching(node -> node.resources().equals(resources)); } + /** Returns the subset of nodes having storage of given type */ + public NodeList storageType(NodeResources.StorageType storageType) { + return matching(node -> node.resources().storageType() == storageType); + } + /** Returns the subset of nodes which satisfy the given resources */ public NodeList satisfies(NodeResources resources) { return matching(node -> node.resources().satisfies(resources)); } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java index 5ffadd806d5..5f43d80b87a 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainer.java @@ -79,6 +79,7 @@ public class DynamicProvisioningMaintainer extends NodeRepositoryMaintainer { NodeList nodes = nodeRepository().nodes().list(); resumeProvisioning(nodes); convergeToCapacity(nodes); + replaceRootDisk(nodes); return 1.0; } @@ -151,6 +152,20 @@ public class DynamicProvisioningMaintainer extends NodeRepositoryMaintainer { }); } + /** Replace the root disk of hosts that have requested soft-rebuild */ + private void replaceRootDisk(NodeList nodes) { + NodeList softRebuildingHosts = nodes.rebuilding(true); + for (var host : softRebuildingHosts) { + Optional<NodeMutex> optionalMutex = nodeRepository().nodes().lockAndGet(host, Optional.of(Duration.ofSeconds(10))); + try (NodeMutex mutex = optionalMutex.get()) { + Node updatedNode = hostProvisioner.replaceRootDisk(host); + if (!updatedNode.status().wantToRebuild()) { + nodeRepository().nodes().write(updatedNode, mutex); + } + } + } + } + /** * Provision hosts to ensure there is room to allocate spare nodes. * diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java index d641f59eafb..ec3e2539170 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Nodes.java @@ -638,30 +638,35 @@ public class Nodes { /** Retire and deprovision given host and all of its children */ public List<Node> deprovision(String hostname, Agent agent, Instant instant) { - return decommission(hostname, DecommissionOperation.deprovision, agent, instant); + return decommission(hostname, HostOperation.deprovision, agent, instant); } - /** Retire and rebuild given host and all of its children */ - public List<Node> rebuild(String hostname, Agent agent, Instant instant) { - return decommission(hostname, DecommissionOperation.rebuild, agent, instant); + /** Rebuild given host */ + public List<Node> rebuild(String hostname, boolean soft, Agent agent, Instant instant) { + return decommission(hostname, soft ? HostOperation.softRebuild : HostOperation.rebuild, agent, instant); } - private List<Node> decommission(String hostname, DecommissionOperation op, Agent agent, Instant instant) { + private List<Node> decommission(String hostname, HostOperation op, Agent agent, Instant instant) { Optional<NodeMutex> nodeMutex = lockAndGet(hostname); if (nodeMutex.isEmpty()) return List.of(); Node host = nodeMutex.get().node(); if (!host.type().isHost()) throw new IllegalArgumentException("Cannot " + op + " non-host " + host); - List<Node> result; - boolean wantToDeprovision = op == DecommissionOperation.deprovision; - boolean wantToRebuild = op == DecommissionOperation.rebuild; + + boolean wantToDeprovision = op == HostOperation.deprovision; + boolean wantToRebuild = op == HostOperation.rebuild || op == HostOperation.softRebuild; + boolean wantToRetire = op.needsRetirement(); + List<Node> result = new ArrayList<>(); try (NodeMutex lock = nodeMutex.get(); Mutex allocationLock = lockUnallocated()) { // This takes allocationLock to prevent any further allocation of nodes on this host host = lock.node(); - result = performOn(list(allocationLock).childrenOf(host), (node, nodeLock) -> { - Node newNode = node.withWantToRetire(true, wantToDeprovision, wantToRebuild, agent, instant); - return write(newNode, nodeLock); - }); - Node newHost = host.withWantToRetire(true, wantToDeprovision, wantToRebuild, agent, instant); + if (wantToRetire) { // Apply recursively if we're retiring + List<Node> updatedNodes = performOn(list(allocationLock).childrenOf(host), (node, nodeLock) -> { + Node newNode = node.withWantToRetire(wantToRetire, wantToDeprovision, wantToRebuild, agent, instant); + return write(newNode, nodeLock); + }); + result.addAll(updatedNodes); + } + Node newHost = host.withWantToRetire(wantToRetire, wantToDeprovision, wantToRebuild, agent, instant); result.add(write(newHost, lock)); } return result; @@ -863,10 +868,28 @@ public class Nodes { retirementRequestedByOperator; } - /** The different ways a host can be decommissioned */ - private enum DecommissionOperation { - deprovision, - rebuild, + private enum HostOperation { + + /** Host is deprovisioned and data is destroyed */ + deprovision(true), + + /** Host is deprovisioned, the same host is later re-provisioned and data is destroyed */ + rebuild(true), + + /** Host is stopped and re-bootstrapped, data is preserved */ + softRebuild(false); + + private final boolean needsRetirement; + + HostOperation(boolean needsRetirement) { + this.needsRetirement = needsRetirement; + } + + /** Returns whether this operation requires the host and its children to be retired */ + public boolean needsRetirement() { + return needsRetirement; + } + } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Status.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Status.java index cc3f610cc44..ef0f899ca3e 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Status.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/node/Status.java @@ -45,8 +45,8 @@ public class Status { if (wantToDeprovision && wantToRebuild) { throw new IllegalArgumentException("Node cannot be marked both wantToDeprovision and wantToRebuild"); } - if ((wantToDeprovision || wantToRebuild) && !wantToRetire) { - throw new IllegalArgumentException("Node cannot be marked wantToDeprovision or wantToRebuild unless it's also marked wantToRetire"); + if (wantToDeprovision && !wantToRetire) { + throw new IllegalArgumentException("Node cannot be marked wantToDeprovision unless it's also marked wantToRetire"); } this.wantToRetire = wantToRetire; this.wantToDeprovision = wantToDeprovision; diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/CompositeOsUpgrader.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/CompositeOsUpgrader.java new file mode 100644 index 00000000000..7aaf37a8ee6 --- /dev/null +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/CompositeOsUpgrader.java @@ -0,0 +1,28 @@ +package com.yahoo.vespa.hosted.provision.os; + +import com.yahoo.config.provision.NodeType; + +import java.util.List; + +/** + * An implementation of {@link OsUpgrader} that delegates calls to multiple implementations. + * + * @author mpolden + */ +public record CompositeOsUpgrader(List<OsUpgrader> upgraders) implements OsUpgrader { + + public CompositeOsUpgrader(List<OsUpgrader> upgraders) { + this.upgraders = List.copyOf(upgraders); + } + + @Override + public void upgradeTo(OsVersionTarget target) { + upgraders.forEach(upgrader -> upgrader.upgradeTo(target)); + } + + @Override + public void disableUpgrade(NodeType type) { + upgraders.forEach(upgrader -> upgrader.disableUpgrade(type)); + } + +} diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersions.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersions.java index 440046ab818..89fdf9d4b2a 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersions.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/OsVersions.java @@ -4,12 +4,15 @@ package com.yahoo.vespa.hosted.provision.os; import com.yahoo.component.Version; import com.yahoo.config.provision.NodeType; import com.yahoo.vespa.curator.Lock; +import com.yahoo.vespa.flags.BooleanFlag; +import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeRepository; import com.yahoo.vespa.hosted.provision.node.Status; import com.yahoo.vespa.hosted.provision.persistence.CuratorDatabaseClient; import java.time.Duration; +import java.util.List; import java.util.Objects; import java.util.Optional; import java.util.function.UnaryOperator; @@ -35,18 +38,20 @@ public class OsVersions { private final NodeRepository nodeRepository; private final CuratorDatabaseClient db; - private final boolean reprovisionToUpgradeOs; + private final boolean dynamicProvisioning; private final int maxDelegatedUpgrades; + private final BooleanFlag softRebuildFlag; public OsVersions(NodeRepository nodeRepository) { - this(nodeRepository, nodeRepository.zone().getCloud().reprovisionToUpgradeOs(), MAX_DELEGATED_UPGRADES); + this(nodeRepository, nodeRepository.zone().getCloud().dynamicProvisioning(), MAX_DELEGATED_UPGRADES); } - OsVersions(NodeRepository nodeRepository, boolean reprovisionToUpgradeOs, int maxDelegatedUpgrades) { + OsVersions(NodeRepository nodeRepository, boolean dynamicProvisioning, int maxDelegatedUpgrades) { this.nodeRepository = Objects.requireNonNull(nodeRepository); this.db = nodeRepository.database(); - this.reprovisionToUpgradeOs = reprovisionToUpgradeOs; + this.dynamicProvisioning = dynamicProvisioning; this.maxDelegatedUpgrades = maxDelegatedUpgrades; + this.softRebuildFlag = Flags.SOFT_REBUILD.bindTo(nodeRepository.flagSource()); // Read and write all versions to make sure they are stored in the latest version of the serialized format try (var lock = db.lockOsVersionChange()) { @@ -136,8 +141,16 @@ public class OsVersions { /** Returns the upgrader to use when upgrading given node type to target */ private OsUpgrader chooseUpgrader(NodeType nodeType, Optional<Version> target) { - if (reprovisionToUpgradeOs) { - return new RetiringOsUpgrader(nodeRepository); + if (dynamicProvisioning) { + boolean softRebuild = softRebuildFlag.value(); + RetiringOsUpgrader retiringOsUpgrader = new RetiringOsUpgrader(nodeRepository, softRebuild); + if (softRebuild) { + // If soft rebuild is enabled, we can use RebuildingOsUpgrader for hosts with remote storage. + // RetiringOsUpgrader is then only used for hosts with local storage. + return new CompositeOsUpgrader(List.of(new RebuildingOsUpgrader(nodeRepository, softRebuild), + retiringOsUpgrader)); + } + return retiringOsUpgrader; } // Require rebuild if we have any nodes of this type on a major version lower than target boolean rebuildRequired = target.isPresent() && @@ -147,7 +160,7 @@ public class OsVersions { .anyMatch(osVersion -> osVersion.current().isPresent() && osVersion.current().get().getMajor() < target.get().getMajor()); if (rebuildRequired) { - return new RebuildingOsUpgrader(nodeRepository); + return new RebuildingOsUpgrader(nodeRepository, false); } return new DelegatingOsUpgrader(nodeRepository, maxDelegatedUpgrades); } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RebuildingOsUpgrader.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RebuildingOsUpgrader.java index f96effe9e10..6b61c864a0c 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RebuildingOsUpgrader.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RebuildingOsUpgrader.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.provision.os; import com.yahoo.component.Version; +import com.yahoo.config.provision.NodeResources; import com.yahoo.config.provision.NodeType; import com.yahoo.vespa.flags.IntFlag; import com.yahoo.vespa.flags.PermanentFlags; @@ -22,10 +23,10 @@ import java.util.Set; import java.util.logging.Logger; /** - * An upgrader that retires and rebuilds hosts on stale OS versions. + * An upgrader that rebuilds hosts on stale OS versions. * - * - We limit the number of concurrent rebuilds to reduce impact of retiring too many hosts. - * - We limit rebuilds by cluster so that at most one node per stateful cluster per application is retired at a time. + * - We limit the number of concurrent rebuilds to reduce impact of suspending or retiring too many hosts. + * - We limit rebuilds by cluster so that at most one node per stateful cluster per application is rebuilt at a time. * * Used in cases where performing an OS upgrade requires rebuilding the host, e.g. when upgrading across major versions. * @@ -37,10 +38,12 @@ public class RebuildingOsUpgrader implements OsUpgrader { private final NodeRepository nodeRepository; private final IntFlag maxRebuilds; + private final boolean softRebuild; - public RebuildingOsUpgrader(NodeRepository nodeRepository) { + public RebuildingOsUpgrader(NodeRepository nodeRepository, boolean softRebuild) { this.nodeRepository = nodeRepository; this.maxRebuilds = PermanentFlags.MAX_REBUILDS.bindTo(nodeRepository.flagSource()); + this.softRebuild = softRebuild; } @Override @@ -59,22 +62,27 @@ public class RebuildingOsUpgrader implements OsUpgrader { private int rebuildLimit(NodeType hostType, NodeList hostsOfType) { if (hostsOfType.stream().anyMatch(host -> host.type() != hostType)) illegal("All hosts must be a " + hostType); int limit = hostType == NodeType.host ? maxRebuilds.value() : 1; - return Math.max(0, limit - hostsOfType.rebuilding().size()); + return Math.max(0, limit - hostsOfType.rebuilding(softRebuild).size()); } private List<Node> rebuildableHosts(OsVersionTarget target, NodeList allNodes, Instant now) { NodeList hostsOfTargetType = allNodes.nodeType(target.nodeType()); + if (softRebuild) { + // Soft rebuild is enabled so this should only act on hosts with remote storage + hostsOfTargetType = hostsOfTargetType.storageType(NodeResources.StorageType.remote); + } int rebuildLimit = rebuildLimit(target.nodeType(), hostsOfTargetType); // Find stateful clusters with retiring nodes NodeList activeNodes = allNodes.state(Node.State.active); Set<ClusterId> retiringClusters = new HashSet<>(activeNodes.nodeType(target.nodeType().childNodeType()) - .retiring().statefulClusters()); + .retiring() + .statefulClusters()); // Rebuild hosts not containing stateful clusters with retiring nodes, up to rebuild limit List<Node> hostsToRebuild = new ArrayList<>(rebuildLimit); NodeList candidates = hostsOfTargetType.state(Node.State.active) - .not().rebuilding() + .not().rebuilding(softRebuild) .osVersionIsBefore(target.version()) .matching(node -> canUpgradeAt(now, node)) .byIncreasingOsVersion(); @@ -91,10 +99,10 @@ public class RebuildingOsUpgrader implements OsUpgrader { } private void rebuild(Node host, Version target, Instant now) { - LOG.info("Retiring and rebuilding " + host + ": On stale OS version " + + LOG.info((softRebuild ? "Soft-rebuilding " : "Retiring and rebuilding ") + host + ": On stale OS version " + host.status().osVersion().current().map(Version::toFullString).orElse("<unset>") + ", want " + target); - nodeRepository.nodes().rebuild(host.hostname(), Agent.RebuildingOsUpgrader, now); + nodeRepository.nodes().rebuild(host.hostname(), softRebuild, Agent.RebuildingOsUpgrader, now); nodeRepository.nodes().upgradeOs(NodeListFilter.from(host), Optional.of(target)); } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RetiringOsUpgrader.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RetiringOsUpgrader.java index 43843f6fe5a..860a17be28c 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RetiringOsUpgrader.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/os/RetiringOsUpgrader.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.provision.os; import com.yahoo.component.Version; +import com.yahoo.config.provision.NodeResources; import com.yahoo.config.provision.NodeType; import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeList; @@ -28,8 +29,11 @@ public class RetiringOsUpgrader implements OsUpgrader { protected final NodeRepository nodeRepository; - public RetiringOsUpgrader(NodeRepository nodeRepository) { + private final boolean softRebuild; + + public RetiringOsUpgrader(NodeRepository nodeRepository, boolean softRebuild) { this.nodeRepository = nodeRepository; + this.softRebuild = softRebuild; } @Override @@ -57,6 +61,10 @@ public class RetiringOsUpgrader implements OsUpgrader { /** Returns nodes that are candidates for upgrade */ private NodeList candidates(Instant instant, OsVersionTarget target, NodeList allNodes) { NodeList activeNodes = allNodes.state(Node.State.active).nodeType(target.nodeType()); + if (softRebuild) { + // Soft rebuild is enabled, so this should only act on hosts with local storage + activeNodes = activeNodes.storageType(NodeResources.StorageType.local); + } if (activeNodes.isEmpty()) return NodeList.of(); Duration nodeBudget = target.upgradeBudget().dividedBy(activeNodes.size()); diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/HostProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/HostProvisioner.java index 567fa9098c9..9b765adca89 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/HostProvisioner.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/provisioning/HostProvisioner.java @@ -79,6 +79,12 @@ public interface HostProvisioner { */ void deprovision(Node host); + /** Replace the root (OS) disk of host. Implementations of this are expected to be idempotent. + * + * @return the updated node object + */ + Node replaceRootDisk(Node host); + /** * Returns the maintenance events scheduled for hosts in this zone, in given cloud accounts. Host events in the * zone's default cloud account are always included. diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodesResponse.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodesResponse.java index 309280c8f15..c82cd8fb47f 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodesResponse.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/restapi/NodesResponse.java @@ -66,11 +66,10 @@ class NodesResponse extends SlimeJsonResponse { Cursor root = slime.setObject(); switch (responseType) { - case nodeList: nodesToSlime(filter.states(), root); break; - case stateList : statesToSlime(root); break; - case nodesInStateList: nodesToSlime(Set.of(NodeSerializer.stateFrom(lastElement(parentUrl))), root); break; - case singleNode : nodeToSlime(lastElement(parentUrl), root); break; - default: throw new IllegalArgumentException(); + case nodeList -> nodesToSlime(filter.states(), root); + case stateList -> statesToSlime(root); + case nodesInStateList -> nodesToSlime(Set.of(NodeSerializer.stateFrom(lastElement(parentUrl))), root); + case singleNode -> nodeToSlime(lastElement(parentUrl), root); } } diff --git a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockHostProvisioner.java b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockHostProvisioner.java index 8d60dd30dd1..13753c12664 100644 --- a/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockHostProvisioner.java +++ b/node-repository/src/main/java/com/yahoo/vespa/hosted/provision/testutils/MockHostProvisioner.java @@ -7,16 +7,18 @@ import com.yahoo.config.provision.CloudAccount; import com.yahoo.config.provision.ClusterSpec; import com.yahoo.config.provision.Flavor; import com.yahoo.config.provision.HostEvent; +import com.yahoo.config.provision.NodeAllocationException; import com.yahoo.config.provision.NodeResources; import com.yahoo.config.provision.NodeType; -import com.yahoo.config.provision.NodeAllocationException; import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.node.Address; +import com.yahoo.vespa.hosted.provision.node.Agent; import com.yahoo.vespa.hosted.provision.node.IP; import com.yahoo.vespa.hosted.provision.provisioning.FatalProvisioningException; import com.yahoo.vespa.hosted.provision.provisioning.HostProvisioner; import com.yahoo.vespa.hosted.provision.provisioning.ProvisionedHost; +import java.time.Instant; import java.util.ArrayList; import java.util.Collections; import java.util.EnumSet; @@ -37,6 +39,7 @@ public class MockHostProvisioner implements HostProvisioner { private final List<Flavor> flavors; private final MockNameResolver nameResolver; private final int memoryTaxGb; + private final Set<String> rebuildsCompleted = new HashSet<>(); private int deprovisionedHosts = 0; private EnumSet<Behaviour> behaviours = EnumSet.noneOf(Behaviour.class); @@ -103,6 +106,16 @@ public class MockHostProvisioner implements HostProvisioner { } @Override + public Node replaceRootDisk(Node host) { + if (!host.type().isHost()) throw new IllegalArgumentException(host + " is not a host"); + if (rebuildsCompleted.remove(host.hostname())) { + return host.withWantToRetire(host.status().wantToRetire(), host.status().wantToDeprovision(), + false, Agent.system, Instant.ofEpochMilli(123)); + } + return host; + } + + @Override public List<HostEvent> hostEventsIn(List<CloudAccount> cloudAccounts) { return Collections.unmodifiableList(hostEvents); } @@ -129,6 +142,11 @@ public class MockHostProvisioner implements HostProvisioner { return this; } + public MockHostProvisioner completeRebuildOf(Node host) { + rebuildsCompleted.add(host.hostname()); + return this; + } + public MockHostProvisioner overrideHostFlavor(String flavorName) { Flavor flavor = flavors.stream().filter(f -> f.name().equals(flavorName)) .findFirst() diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java index e5e361da379..72b49a4794a 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/maintenance/DynamicProvisioningMaintainerTest.java @@ -603,6 +603,30 @@ public class DynamicProvisioningMaintainerTest { } } + @Test + public void rebuild_host() { + var tester = new DynamicProvisioningTester(); + Node host1 = tester.addNode("host1", Optional.empty(), NodeType.host, Node.State.active); + Node host11 = tester.addNode("host1-1", Optional.of("host1"), NodeType.tenant, Node.State.parked, DynamicProvisioningTester.tenantApp); + Node host2 = tester.addNode("host2", Optional.empty(), NodeType.host, Node.State.active); + Node host21 = tester.addNode("host2-1", Optional.of("host2"), NodeType.tenant, Node.State.parked, DynamicProvisioningTester.tenantApp); + + // No rebuilds in initial run + tester.maintainer.maintain(); + assertEquals(0, tester.nodeRepository.nodes().list().rebuilding(true).size()); + + // Host starts rebuilding + tester.nodeRepository.nodes().rebuild(host1.hostname(), true, Agent.RebuildingOsUpgrader, + tester.nodeRepository.clock().instant()); + tester.maintainer.maintain(); + assertEquals(1, tester.nodeRepository.nodes().list().rebuilding(true).size()); + + // Rebuild completes + tester.hostProvisioner.completeRebuildOf(host1); + tester.maintainer.maintain(); + assertEquals(0, tester.nodeRepository.nodes().list().rebuilding(true).size()); + } + private void assertCfghost3IsActive(DynamicProvisioningTester tester) { assertEquals(5, tester.nodeRepository.nodes().list(Node.State.active).size()); assertEquals(3, tester.nodeRepository.nodes().list(Node.State.active).nodeType(NodeType.confighost).size()); diff --git a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java index 3d7db9a1f96..4d75b8a5acc 100644 --- a/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java +++ b/node-repository/src/test/java/com/yahoo/vespa/hosted/provision/os/OsVersionsTest.java @@ -8,6 +8,7 @@ import com.yahoo.config.provision.HostSpec; import com.yahoo.config.provision.NodeResources; import com.yahoo.config.provision.NodeType; import com.yahoo.test.ManualClock; +import com.yahoo.vespa.flags.Flags; import com.yahoo.vespa.flags.PermanentFlags; import com.yahoo.vespa.hosted.provision.Node; import com.yahoo.vespa.hosted.provision.NodeList; @@ -273,35 +274,35 @@ public class OsVersionsTest { versions.resumeUpgradeOf(NodeType.host, true); // One host starts rebuilding - assertEquals(1, hostNodes.get().rebuilding().size()); + assertEquals(1, hostNodes.get().rebuilding(false).size()); // We cannot rebuild another host until the current one is done versions.resumeUpgradeOf(NodeType.host, true); - NodeList hostsRebuilding = hostNodes.get().rebuilding(); + NodeList hostsRebuilding = hostNodes.get().rebuilding(false); assertEquals(1, hostsRebuilding.size()); completeRebuildOf(hostsRebuilding.asList(), NodeType.host); assertEquals(1, hostNodes.get().onOsVersion(version1).size()); // Second host is rebuilt versions.resumeUpgradeOf(NodeType.host, true); - completeRebuildOf(hostNodes.get().rebuilding().asList(), NodeType.host); + completeRebuildOf(hostNodes.get().rebuilding(false).asList(), NodeType.host); assertEquals(2, hostNodes.get().onOsVersion(version1).size()); // The remaining hosts complete their upgrade for (int i = 0; i < hostCount - 2; i++) { versions.resumeUpgradeOf(NodeType.host, true); - hostsRebuilding = hostNodes.get().rebuilding(); + hostsRebuilding = hostNodes.get().rebuilding(false); assertEquals(1, hostsRebuilding.size()); completeRebuildOf(hostsRebuilding.asList(), NodeType.host); } // All hosts upgraded and none are rebuilding - assertEquals(hostCount, hostNodes.get().onOsVersion(version1).not().rebuilding().size()); + assertEquals(hostCount, hostNodes.get().onOsVersion(version1).not().rebuilding(false).size()); assertEquals(hostCount, tester.nodeRepository().nodes().list(Node.State.active).size()); // Resuming after everything has upgraded has no effect versions.resumeUpgradeOf(NodeType.host, true); - assertEquals(0, hostNodes.get().rebuilding().size()); + assertEquals(0, hostNodes.get().rebuilding(false).size()); // Next version is within same major. Upgrade mechanism switches to delegated var version2 = Version.fromString("8.1"); @@ -319,12 +320,62 @@ public class OsVersionsTest { // Resuming upgrades reactivated host. Upgrade mechanism switches to rebuilding versions.resumeUpgradeOf(NodeType.host, true); - hostsRebuilding = hostNodes.get().rebuilding(); + hostsRebuilding = hostNodes.get().rebuilding(false); assertEquals(List.of(reactivatedHost), hostsRebuilding.asList()); completeRebuildOf(hostsRebuilding.asList(), NodeType.host); } @Test + public void upgrade_by_soft_rebuilding() { + int maxRebuilds = 3; + int hostCount = 12; + boolean softRebuild = true; + + tester.flagSource().withIntFlag(PermanentFlags.MAX_REBUILDS.id(), maxRebuilds); + tester.flagSource().withBooleanFlag(Flags.SOFT_REBUILD.id(), softRebuild); + var versions = new OsVersions(tester.nodeRepository(), true, Integer.MAX_VALUE); + + provisionInfraApplication(hostCount, infraApplication, NodeType.host, NodeResources.StorageType.remote); + Supplier<NodeList> hostNodes = () -> tester.nodeRepository().nodes().list().nodeType(NodeType.host); + + // New target is set + int hostsRebuilt = 0; + var version1 = Version.fromString("8.0"); + versions.setTarget(NodeType.host, version1, Duration.ZERO, false); + versions.resumeUpgradeOf(NodeType.host, true); + + // First batch of hosts start rebuilding + assertEquals(maxRebuilds, hostNodes.get().rebuilding(softRebuild).size()); + + // We cannot rebuild another host yet + versions.resumeUpgradeOf(NodeType.host, true); + NodeList hostsRebuilding = hostNodes.get().rebuilding(softRebuild); + assertEquals(maxRebuilds, hostsRebuilding.size()); + completeSoftRebuildOf(hostsRebuilding.asList()); + assertEquals(hostsRebuilt += maxRebuilds, hostNodes.get().onOsVersion(version1).size()); + + // Another batch is rebuilt + versions.resumeUpgradeOf(NodeType.host, true); + completeSoftRebuildOf(hostNodes.get().rebuilding(softRebuild).asList()); + assertEquals(hostsRebuilt += maxRebuilds, hostsRebuilt); + + // The remaining batches complete their upgrade + for (int i = 0; i < (hostCount - hostsRebuilt) / maxRebuilds; i++) { + versions.resumeUpgradeOf(NodeType.host, true); + hostsRebuilding = hostNodes.get().rebuilding(softRebuild); + assertEquals(maxRebuilds, hostsRebuilding.size()); + completeSoftRebuildOf(hostsRebuilding.asList()); + } + + // All hosts upgraded and none are rebuilding + assertEquals(hostCount, hostNodes.get().onOsVersion(version1).not().rebuilding(softRebuild).size()); + + // Resuming after everything has upgraded has no effect + versions.resumeUpgradeOf(NodeType.host, true); + assertEquals(0, hostNodes.get().rebuilding(softRebuild).size()); + } + + @Test public void upgrade_by_rebuilding_multiple_host_types() { tester.flagSource().withIntFlag(PermanentFlags.MAX_REBUILDS.id(), 1); var versions = new OsVersions(tester.nodeRepository(), false, Integer.MAX_VALUE); @@ -349,7 +400,7 @@ public class OsVersionsTest { for (int i = 0; i < hostCount; i++) { versions.resumeUpgradeOf(NodeType.host, true); versions.resumeUpgradeOf(NodeType.confighost, true); - NodeList hostsRebuilding = hosts.get().rebuilding(); + NodeList hostsRebuilding = hosts.get().rebuilding(false); assertEquals(2, hostsRebuilding.size()); completeRebuildOf(hostsRebuilding.nodeType(NodeType.host).asList(), NodeType.host); completeRebuildOf(hostsRebuilding.nodeType(NodeType.confighost).asList(), NodeType.confighost); @@ -382,7 +433,7 @@ public class OsVersionsTest { versions.resumeUpgradeOf(NodeType.host, true); NodeList allNodes = tester.nodeRepository().nodes().list(); List<Node> hostsRebuilding = allNodes.nodeType(NodeType.host) - .rebuilding() + .rebuilding(false) .sortedBy(Comparator.comparing(Node::hostname)) .asList(); List<Optional<ApplicationId>> owners = List.of(Optional.of(app1), Optional.of(app2), Optional.empty()); @@ -420,7 +471,7 @@ public class OsVersionsTest { // Since both applications now occupy all remaining hosts, we can only upgrade 1 at a time for (int i = 0; i < hostsOnOldVersion.size(); i++) { versions.resumeUpgradeOf(NodeType.host, true); - hostsRebuilding = hosts.get().rebuilding().asList(); + hostsRebuilding = hosts.get().rebuilding(false).asList(); assertEquals(1, hostsRebuilding.size()); replaceNodes(app1); replaceNodes(app2); @@ -430,7 +481,7 @@ public class OsVersionsTest { // Resuming upgrade has no effect as all hosts have upgraded versions.resumeUpgradeOf(NodeType.host, true); NodeList allHosts = hosts.get(); - assertEquals(0, allHosts.rebuilding().size()); + assertEquals(0, allHosts.rebuilding(false).size()); assertEquals(allHosts.size(), allHosts.onOsVersion(version1).size()); } @@ -454,7 +505,7 @@ public class OsVersionsTest { // Upgrades 1 infrastructure host at a time for (int i = 0; i < hostCount; i++) { versions.resumeUpgradeOf(NodeType.proxyhost, true); - List<Node> hostsRebuilding = hosts.get().rebuilding().asList(); + List<Node> hostsRebuilding = hosts.get().rebuilding(false).asList(); assertEquals(1, hostsRebuilding.size()); completeRebuildOf(hostsRebuilding, NodeType.proxyhost); } @@ -490,7 +541,13 @@ public class OsVersionsTest { } private List<Node> provisionInfraApplication(int nodeCount, ApplicationId application, NodeType nodeType) { - var nodes = tester.makeReadyNodes(nodeCount, new NodeResources(48, 128, 2000, 10), nodeType, 10); + return provisionInfraApplication(nodeCount, application, nodeType, NodeResources.StorageType.local); + } + + private List<Node> provisionInfraApplication(int nodeCount, ApplicationId application, NodeType nodeType, NodeResources.StorageType storageType) { + var nodes = tester.makeReadyNodes(nodeCount, new NodeResources(48, 128, 2000, 10, + NodeResources.DiskSpeed.fast, storageType), + nodeType, 10); tester.prepareAndActivateInfraApplication(application, nodeType); return nodes.stream() .map(Node::hostname) @@ -557,4 +614,15 @@ public class OsVersionsTest { }); } + private void completeSoftRebuildOf(List<Node> nodes) { + tester.patchNodes(nodes, (node) -> { + Optional<Version> wantedOsVersion = node.status().osVersion().wanted(); + assertFalse(node + " is not retiring", node.status().wantToRetire()); + assertTrue(node + " is rebuilding", node.status().wantToRebuild()); + node = node.withWantToRetire(false, false, false, Agent.system, + tester.clock().instant()); + return node.with(node.status().withOsVersion(node.status().osVersion().withCurrent(wantedOsVersion))); + }); + } + } diff --git a/searchcore/src/tests/proton/matching/matching_test.cpp b/searchcore/src/tests/proton/matching/matching_test.cpp index df8f2a9476b..70f6cf73a72 100644 --- a/searchcore/src/tests/proton/matching/matching_test.cpp +++ b/searchcore/src/tests/proton/matching/matching_test.cpp @@ -934,8 +934,8 @@ TEST("require that getSummaryFeatures prefers cached query setup") { } TEST("require that match params are set up straight with ranking on") { - MatchParams p(1, 2, 4, 0.7, 0, 1, true, true); - ASSERT_EQUAL(1u, p.numDocs); + MatchParams p(10, 2, 4, 0.7, 0, 1, true, true); + ASSERT_EQUAL(10u, p.numDocs); ASSERT_EQUAL(2u, p.heapSize); ASSERT_EQUAL(4u, p.arraySize); ASSERT_EQUAL(0.7, p.rankDropLimit); @@ -945,8 +945,8 @@ TEST("require that match params are set up straight with ranking on") { } TEST("require that match params can turn off rank-drop-limit") { - MatchParams p(1, 2, 4, -std::numeric_limits<feature_t>::quiet_NaN(), 0, 1, true, true); - ASSERT_EQUAL(1u, p.numDocs); + MatchParams p(10, 2, 4, -std::numeric_limits<feature_t>::quiet_NaN(), 0, 1, true, true); + ASSERT_EQUAL(10u, p.numDocs); ASSERT_EQUAL(2u, p.heapSize); ASSERT_EQUAL(4u, p.arraySize); ASSERT_TRUE(std::isnan(p.rankDropLimit)); @@ -957,8 +957,8 @@ TEST("require that match params can turn off rank-drop-limit") { TEST("require that match params are set up straight with ranking on arraySize is atleast the size of heapSize") { - MatchParams p(1, 6, 4, 0.7, 1, 1, true, true); - ASSERT_EQUAL(1u, p.numDocs); + MatchParams p(10, 6, 4, 0.7, 1, 1, true, true); + ASSERT_EQUAL(10u, p.numDocs); ASSERT_EQUAL(6u, p.heapSize); ASSERT_EQUAL(6u, p.arraySize); ASSERT_EQUAL(0.7, p.rankDropLimit); @@ -967,8 +967,8 @@ TEST("require that match params are set up straight with ranking on arraySize is } TEST("require that match params are set up straight with ranking on arraySize is atleast the size of hits+offset") { - MatchParams p(1, 6, 4, 0.7, 4, 4, true, true); - ASSERT_EQUAL(1u, p.numDocs); + MatchParams p(10, 6, 4, 0.7, 4, 4, true, true); + ASSERT_EQUAL(10u, p.numDocs); ASSERT_EQUAL(6u, p.heapSize); ASSERT_EQUAL(8u, p.arraySize); ASSERT_EQUAL(0.7, p.rankDropLimit); @@ -976,9 +976,29 @@ TEST("require that match params are set up straight with ranking on arraySize is ASSERT_EQUAL(4u, p.hits); } -TEST("require that match params are set up straight with ranking off array and heap size is 0") { - MatchParams p(1, 6, 4, 0.7, 4, 4, true, false); +TEST("require that match params are capped by numDocs") { + MatchParams p(1, 6, 4, 0.7, 4, 4, true, true); ASSERT_EQUAL(1u, p.numDocs); + ASSERT_EQUAL(1u, p.heapSize); + ASSERT_EQUAL(1u, p.arraySize); + ASSERT_EQUAL(0.7, p.rankDropLimit); + ASSERT_EQUAL(1u, p.offset); + ASSERT_EQUAL(0u, p.hits); +} + +TEST("require that match params are capped by numDocs and hits adjusted down") { + MatchParams p(5, 6, 4, 0.7, 4, 4, true, true); + ASSERT_EQUAL(5u, p.numDocs); + ASSERT_EQUAL(5u, p.heapSize); + ASSERT_EQUAL(5u, p.arraySize); + ASSERT_EQUAL(0.7, p.rankDropLimit); + ASSERT_EQUAL(4u, p.offset); + ASSERT_EQUAL(1u, p.hits); +} + +TEST("require that match params are set up straight with ranking off array and heap size is 0") { + MatchParams p(10, 6, 4, 0.7, 4, 4, true, false); + ASSERT_EQUAL(10u, p.numDocs); ASSERT_EQUAL(0u, p.heapSize); ASSERT_EQUAL(0u, p.arraySize); ASSERT_EQUAL(0.7, p.rankDropLimit); diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp b/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp index 7465088d8f0..68a6f74fa50 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp @@ -8,7 +8,8 @@ namespace proton::matching { namespace { -uint32_t computeArraySize(uint32_t hitsPlussOffset, uint32_t heapSize, uint32_t arraySize) +uint32_t +computeArraySize(uint32_t hitsPlussOffset, uint32_t heapSize, uint32_t arraySize) { return std::max(hitsPlussOffset, std::max(heapSize, arraySize)); } @@ -24,16 +25,17 @@ MatchParams::MatchParams(uint32_t numDocs_in, bool hasFinalRank, bool needRanking) : numDocs(numDocs_in), - heapSize((hasFinalRank && needRanking) ? heapSize_in : 0), + heapSize((hasFinalRank && needRanking) ? std::min(numDocs_in, heapSize_in) : 0), arraySize((needRanking && ((heapSize_in + arraySize_in) > 0)) - ? computeArraySize(hits_in + offset_in, heapSize, arraySize_in) + ? std::min(numDocs_in, computeArraySize(hits_in + offset_in, heapSize, arraySize_in)) : 0), - offset(offset_in), - hits(hits_in), + offset(std::min(numDocs_in, offset_in)), + hits(std::min(numDocs_in - offset, hits_in)), rankDropLimit(rankDropLimit_in) { } -bool MatchParams::has_rank_drop_limit() const { +bool +MatchParams::has_rank_drop_limit() const { return ! std::isnan(rankDropLimit); } diff --git a/searchlib/src/tests/memoryindex/datastore/feature_store_test.cpp b/searchlib/src/tests/memoryindex/datastore/feature_store_test.cpp index 34f9f7d27a9..564824031a6 100644 --- a/searchlib/src/tests/memoryindex/datastore/feature_store_test.cpp +++ b/searchlib/src/tests/memoryindex/datastore/feature_store_test.cpp @@ -90,8 +90,7 @@ TEST_F(FeatureStoreTest, features_can_be_added_and_retrieved) r = fs.addFeatures(0, f); r1 = r.first; EXPECT_TRUE(r.second > 0); - EXPECT_EQ(FeatureStore::RefType::align(1u), - FeatureStore::RefType(r1).offset()); + EXPECT_EQ(1u, FeatureStore::RefType(r1).offset()); EXPECT_EQ(0u, FeatureStore::RefType(r1).bufferId()); LOG(info, "bits(%" PRIu64 "), ref.offset(%zu), ref.bufferId(%u)", @@ -131,8 +130,7 @@ TEST_F(FeatureStoreTest, next_words_are_working) r = fs.addFeatures(0, f); r1 = r.first; EXPECT_TRUE(r.second > 0); - EXPECT_EQ(FeatureStore::RefType::align(1u), - FeatureStore::RefType(r1).offset()); + EXPECT_EQ(1u, FeatureStore::RefType(r1).offset()); EXPECT_EQ(0u, FeatureStore::RefType(r1).bufferId()); LOG(info, "bits(%" PRIu64 "), ref.offset(%zu), ref.bufferId(%u)", diff --git a/searchlib/src/tests/memoryindex/datastore/word_store_test.cpp b/searchlib/src/tests/memoryindex/datastore/word_store_test.cpp index 698780a1dc2..1ca87467fc6 100644 --- a/searchlib/src/tests/memoryindex/datastore/word_store_test.cpp +++ b/searchlib/src/tests/memoryindex/datastore/word_store_test.cpp @@ -18,14 +18,14 @@ TEST(WordStoreTest, words_can_be_added_and_retrieved) EntryRef r1 = ws.addWord(w1); EntryRef r2 = ws.addWord(w2); EntryRef r3 = ws.addWord(w3); - uint32_t invp = WordStore::RefType::align(1); // Reserved as invalid + uint32_t invp = WordStore::buffer_array_size; // Reserved as invalid uint32_t w1s = w1.size() + 1; - uint32_t w1p = WordStore::RefType::pad(w1s); + uint32_t w1p = WordStore::calc_pad(w1s); uint32_t w2s = w2.size() + 1; - uint32_t w2p = WordStore::RefType::pad(w2s); - EXPECT_EQ(invp, WordStore::RefType(r1).offset()); - EXPECT_EQ(invp + w1s + w1p, WordStore::RefType(r2).offset()); - EXPECT_EQ(invp + w1s + w1p + w2s + w2p, WordStore::RefType(r3).offset()); + uint32_t w2p = WordStore::calc_pad(w2s); + EXPECT_EQ(invp, WordStore::RefType(r1).offset() * WordStore::buffer_array_size); + EXPECT_EQ(invp + w1s + w1p, WordStore::RefType(r2).offset() * WordStore::buffer_array_size); + EXPECT_EQ(invp + w1s + w1p + w2s + w2p, WordStore::RefType(r3).offset() * WordStore::buffer_array_size); EXPECT_EQ(0u, WordStore::RefType(r1).bufferId()); EXPECT_EQ(0u, WordStore::RefType(r2).bufferId()); EXPECT_EQ(0u, WordStore::RefType(r3).bufferId()); diff --git a/searchlib/src/vespa/searchlib/engine/searchrequest.h b/searchlib/src/vespa/searchlib/engine/searchrequest.h index 7b598735b42..ec1bc7550b5 100644 --- a/searchlib/src/vespa/searchlib/engine/searchrequest.h +++ b/searchlib/src/vespa/searchlib/engine/searchrequest.h @@ -23,7 +23,7 @@ public: SearchRequest(); explicit SearchRequest(RelativeTime relativeTime); - ~SearchRequest(); + ~SearchRequest() override; }; } diff --git a/searchlib/src/vespa/searchlib/memoryindex/feature_store.cpp b/searchlib/src/vespa/searchlib/memoryindex/feature_store.cpp index b37300375a8..b5810d06047 100644 --- a/searchlib/src/vespa/searchlib/memoryindex/feature_store.cpp +++ b/searchlib/src/vespa/searchlib/memoryindex/feature_store.cpp @@ -9,6 +9,7 @@ namespace search::memoryindex { constexpr size_t MIN_BUFFER_ARRAYS = 1024u; using index::SchemaUtil; +using vespalib::datastore::EntryRef; uint64_t FeatureStore::writeFeatures(uint32_t packedIndex, const DocIdAndFeatures &features) @@ -26,10 +27,10 @@ FeatureStore::writeFeatures(uint32_t packedIndex, const DocIdAndFeatures &featur return oldOffset; } -vespalib::datastore::EntryRef +EntryRef FeatureStore::addFeatures(const uint8_t *src, uint64_t byteLen) { - uint32_t pad = RefType::pad(byteLen); + uint32_t pad = calc_pad(byteLen); auto result = _store.rawAllocator<uint8_t>(_typeId).alloc(byteLen + pad, DECODE_SAFETY); uint8_t *dst = result.data; memcpy(dst, src, byteLen); @@ -42,7 +43,7 @@ FeatureStore::addFeatures(const uint8_t *src, uint64_t byteLen) return result.ref; } -std::pair<vespalib::datastore::EntryRef, uint64_t> +std::pair<EntryRef, uint64_t> FeatureStore::addFeatures(uint64_t beginOffset, uint64_t endOffset) { uint64_t bitLen = (endOffset - beginOffset); @@ -52,18 +53,18 @@ FeatureStore::addFeatures(uint64_t beginOffset, uint64_t endOffset) assert(wordLen > 0); assert(byteLen > 0); const uint8_t *src = reinterpret_cast<const uint8_t *>(_f._valI - wordLen); - RefType ref = addFeatures(src, byteLen); + EntryRef ref = addFeatures(src, byteLen); return std::make_pair(ref, bitLen); } -vespalib::datastore::EntryRef -FeatureStore::moveFeatures(vespalib::datastore::EntryRef ref, uint64_t bitLen) +EntryRef +FeatureStore::moveFeatures(EntryRef ref, uint64_t bitLen) { const uint8_t *src = getBits(ref); uint64_t byteLen = (bitLen + 7) / 8; - RefType newRef = addFeatures(src, byteLen); + EntryRef newRef = addFeatures(src, byteLen); // Mark old features as dead - _store.incDead(ref, byteLen + RefType::pad(byteLen)); + _store.incDead(ref, byteLen + calc_pad(byteLen)); return newRef; } @@ -74,8 +75,7 @@ FeatureStore::FeatureStore(const Schema &schema) _d(nullptr), _fieldsParams(), _schema(schema), - _type(RefType::align(1u), MIN_BUFFER_ARRAYS, - RefType::offsetSize() / RefType::align(1u)), + _type(buffer_array_size, MIN_BUFFER_ARRAYS, RefType::offsetSize()), _typeId(0) { _f.setWriteContext(&_fctx); @@ -96,7 +96,7 @@ FeatureStore::~FeatureStore() _store.dropBuffers(); } -std::pair<vespalib::datastore::EntryRef, uint64_t> +std::pair<EntryRef, uint64_t> FeatureStore::addFeatures(uint32_t packedIndex, const DocIdAndFeatures &features) { uint64_t oldOffset = writeFeatures(packedIndex, features); @@ -109,14 +109,14 @@ void FeatureStore::add_features_guard_bytes() { uint32_t len = DECODE_SAFETY; - uint32_t pad = RefType::pad(len); - auto result = _store.rawAllocator<int8_t>(_typeId).alloc(len + pad); + uint32_t pad = calc_pad(len); + auto result = _store.rawAllocator<uint8_t>(_typeId).alloc(len + pad); memset(result.data, 0, len + pad); _store.incDead(result.ref, len + pad); } void -FeatureStore::getFeatures(uint32_t packedIndex, vespalib::datastore::EntryRef ref, DocIdAndFeatures &features) +FeatureStore::getFeatures(uint32_t packedIndex, EntryRef ref, DocIdAndFeatures &features) { setupForField(packedIndex, _d); setupForReadFeatures(ref, _d); @@ -124,7 +124,7 @@ FeatureStore::getFeatures(uint32_t packedIndex, vespalib::datastore::EntryRef re } size_t -FeatureStore::bitSize(uint32_t packedIndex, vespalib::datastore::EntryRef ref) +FeatureStore::bitSize(uint32_t packedIndex, EntryRef ref) { setupForField(packedIndex, _d); setupForUnpackFeatures(ref, _d); @@ -136,8 +136,8 @@ FeatureStore::bitSize(uint32_t packedIndex, vespalib::datastore::EntryRef ref) return bitLen; } -vespalib::datastore::EntryRef -FeatureStore::moveFeatures(uint32_t packedIndex, vespalib::datastore::EntryRef ref) +EntryRef +FeatureStore::moveFeatures(uint32_t packedIndex, EntryRef ref) { uint64_t bitLen = bitSize(packedIndex, ref); return moveFeatures(ref, bitLen); diff --git a/searchlib/src/vespa/searchlib/memoryindex/feature_store.h b/searchlib/src/vespa/searchlib/memoryindex/feature_store.h index a96ae9a8f2d..b1d975d0926 100644 --- a/searchlib/src/vespa/searchlib/memoryindex/feature_store.h +++ b/searchlib/src/vespa/searchlib/memoryindex/feature_store.h @@ -14,11 +14,14 @@ namespace search::memoryindex { */ class FeatureStore { public: - using DataStoreType = vespalib::datastore::DataStoreT<vespalib::datastore::AlignedEntryRefT<22, 2>>; + using DataStoreType = vespalib::datastore::DataStoreT<vespalib::datastore::EntryRefT<22>>; using RefType = DataStoreType::RefType; using EncodeContext = bitcompression::EG2PosOccEncodeContext<true>; using DecodeContextCooked = bitcompression::EG2PosOccDecodeContextCooked<true>; using generation_t = vespalib::GenerationHandler::generation_t; + static constexpr uint32_t buffer_array_size = 4u; // Must be a power of 2 + static constexpr uint32_t pad_constant = buffer_array_size - 1u; + static uint32_t calc_pad(uint32_t val) { return (-val & pad_constant); } private: using Schema = index::Schema; @@ -154,7 +157,7 @@ public: uint32_t bufferId = RefType(ref).bufferId(); const vespalib::datastore::BufferState &state = _store.getBufferState(bufferId); decoder.setEnd( - ((_store.getEntry<uint8_t>(RefType(0, bufferId)) + state.size() - + ((_store.getEntryArray<uint8_t>(RefType(0, bufferId), buffer_array_size) + state.size() - bits) + 7) / 8, false); } @@ -188,7 +191,7 @@ public: */ const uint8_t *getBits(vespalib::datastore::EntryRef ref) const { RefType iRef(ref); - return _store.getEntry<uint8_t>(iRef); + return _store.getEntryArray<uint8_t>(iRef, buffer_array_size); } /** diff --git a/searchlib/src/vespa/searchlib/memoryindex/word_store.cpp b/searchlib/src/vespa/searchlib/memoryindex/word_store.cpp index e5ec4ab7808..441587eb718 100644 --- a/searchlib/src/vespa/searchlib/memoryindex/word_store.cpp +++ b/searchlib/src/vespa/searchlib/memoryindex/word_store.cpp @@ -10,16 +10,13 @@ constexpr size_t MIN_BUFFER_ARRAYS = 1024; WordStore::WordStore() : _store(), _numWords(0), - _type(RefType::align(1), - MIN_BUFFER_ARRAYS, - RefType::offsetSize() / RefType::align(1)), + _type(buffer_array_size, MIN_BUFFER_ARRAYS, RefType::offsetSize()), _typeId(0) { _store.addType(&_type); _store.init_primary_buffers(); } - WordStore::~WordStore() { _store.dropBuffers(); @@ -29,7 +26,7 @@ vespalib::datastore::EntryRef WordStore::addWord(const vespalib::stringref word) { size_t wordSize = word.size() + 1; - size_t bufferSize = RefType::align(wordSize); + size_t bufferSize = wordSize + calc_pad(wordSize); auto result = _store.rawAllocator<char>(_typeId).alloc(bufferSize); char *be = result.data; for (size_t i = 0; i < word.size(); ++i) { diff --git a/searchlib/src/vespa/searchlib/memoryindex/word_store.h b/searchlib/src/vespa/searchlib/memoryindex/word_store.h index b27ae65d776..913f6bc3ea5 100644 --- a/searchlib/src/vespa/searchlib/memoryindex/word_store.h +++ b/searchlib/src/vespa/searchlib/memoryindex/word_store.h @@ -9,8 +9,11 @@ namespace search::memoryindex { class WordStore { public: - using DataStoreType = vespalib::datastore::DataStoreT<vespalib::datastore::AlignedEntryRefT<22, 2>>; + using DataStoreType = vespalib::datastore::DataStoreT<vespalib::datastore::EntryRefT<22>>; using RefType = DataStoreType::RefType; + static constexpr uint32_t buffer_array_size = 4u; // Must be a power of 2 + static constexpr uint32_t pad_constant = buffer_array_size - 1u; + static uint32_t calc_pad(uint32_t val) { return (-val & pad_constant); } private: DataStoreType _store; @@ -24,7 +27,7 @@ public: vespalib::datastore::EntryRef addWord(const vespalib::stringref word); const char *getWord(vespalib::datastore::EntryRef ref) const { RefType internalRef(ref); - return _store.getEntry<char>(internalRef); + return _store.getEntryArray<char>(internalRef, buffer_array_size); } vespalib::MemoryUsage getMemoryUsage() const { diff --git a/searchlib/src/vespa/searchlib/tensor/serialized_fast_value_attribute.cpp b/searchlib/src/vespa/searchlib/tensor/serialized_fast_value_attribute.cpp index 2233eb77e89..3e9f41c812c 100644 --- a/searchlib/src/vespa/searchlib/tensor/serialized_fast_value_attribute.cpp +++ b/searchlib/src/vespa/searchlib/tensor/serialized_fast_value_attribute.cpp @@ -48,13 +48,7 @@ SerializedFastValueAttribute::getTensor(DocId docId) const if (docId < getCommittedDocIdLimit()) { ref = acquire_entry_ref(docId); } - if (!ref.valid()) { - return {}; - } - if (const auto * ptr = _streamedValueStore.get_tensor_entry(ref)) { - return ptr->create_fast_value_view(_tensor_type); - } - return {}; + return _streamedValueStore.get_tensor(ref); } bool diff --git a/searchlib/src/vespa/searchlib/tensor/streamed_value_saver.cpp b/searchlib/src/vespa/searchlib/tensor/streamed_value_saver.cpp index f7b93654c33..25d3901d761 100644 --- a/searchlib/src/vespa/searchlib/tensor/streamed_value_saver.cpp +++ b/searchlib/src/vespa/searchlib/tensor/streamed_value_saver.cpp @@ -31,7 +31,7 @@ StreamedValueSaver::onSave(IAttributeSaveTarget &saveTarget) const uint32_t docIdLimit(_refs.size()); vespalib::nbostream stream; for (uint32_t lid = 0; lid < docIdLimit; ++lid) { - if (_tensorStore.encode_tensor(_refs[lid], stream)) { + if (_tensorStore.encode_stored_tensor(_refs[lid], stream)) { uint32_t sz = stream.size(); datWriter->write(&sz, sizeof(sz)); datWriter->write(stream.peek(), stream.size()); diff --git a/searchlib/src/vespa/searchlib/tensor/streamed_value_store.cpp b/searchlib/src/vespa/searchlib/tensor/streamed_value_store.cpp index 58e625e6aca..763486f82e2 100644 --- a/searchlib/src/vespa/searchlib/tensor/streamed_value_store.cpp +++ b/searchlib/src/vespa/searchlib/tensor/streamed_value_store.cpp @@ -204,6 +204,15 @@ StreamedValueStore::get_tensor_entry(EntryRef ref) const return entry.get(); } +std::unique_ptr<vespalib::eval::Value> +StreamedValueStore::get_tensor(EntryRef ref) const +{ + if (const auto * ptr = get_tensor_entry(ref)) { + return ptr->create_fast_value_view(_tensor_type); + } + return {}; +} + void StreamedValueStore::holdTensor(EntryRef ref) { @@ -229,7 +238,7 @@ StreamedValueStore::move(EntryRef ref) } bool -StreamedValueStore::encode_tensor(EntryRef ref, vespalib::nbostream &target) const +StreamedValueStore::encode_stored_tensor(EntryRef ref, vespalib::nbostream &target) const { if (const auto * entry = get_tensor_entry(ref)) { entry->encode_value(_tensor_type, target); diff --git a/searchlib/src/vespa/searchlib/tensor/streamed_value_store.h b/searchlib/src/vespa/searchlib/tensor/streamed_value_store.h index 29201dc0e61..9c5c5a91d18 100644 --- a/searchlib/src/vespa/searchlib/tensor/streamed_value_store.h +++ b/searchlib/src/vespa/searchlib/tensor/streamed_value_store.h @@ -60,6 +60,7 @@ private: TensorStoreType _concrete_store; const vespalib::eval::ValueType _tensor_type; EntryRef add_entry(TensorEntry::SP tensor); + const TensorEntry* get_tensor_entry(EntryRef ref) const; public: StreamedValueStore(const vespalib::eval::ValueType &tensor_type); ~StreamedValueStore() override; @@ -69,8 +70,8 @@ public: void holdTensor(EntryRef ref) override; EntryRef move(EntryRef ref) override; - const TensorEntry * get_tensor_entry(EntryRef ref) const; - bool encode_tensor(EntryRef ref, vespalib::nbostream &target) const; + std::unique_ptr<vespalib::eval::Value> get_tensor(EntryRef ref) const; + bool encode_stored_tensor(EntryRef ref, vespalib::nbostream &target) const; EntryRef store_tensor(const vespalib::eval::Value &tensor); EntryRef store_encoded_tensor(vespalib::nbostream &encoded); diff --git a/searchlib/src/vespa/searchlib/tensor/tensor_buffer_store.cpp b/searchlib/src/vespa/searchlib/tensor/tensor_buffer_store.cpp index 6c1b3bbd1ee..34454e9f780 100644 --- a/searchlib/src/vespa/searchlib/tensor/tensor_buffer_store.cpp +++ b/searchlib/src/vespa/searchlib/tensor/tensor_buffer_store.cpp @@ -29,8 +29,7 @@ TensorBufferStore::TensorBufferStore(const ValueType& tensor_type, std::shared_p _array_store(ArrayStoreType::optimizedConfigForHugePage(max_small_subspaces_type_id, TensorBufferTypeMapper(max_small_subspaces_type_id, &_ops), MemoryAllocator::HUGEPAGE_SIZE, 4_Ki, 8_Ki, ALLOC_GROW_FACTOR), - std::move(allocator), TensorBufferTypeMapper(max_small_subspaces_type_id, &_ops)), - _add_buffer() + std::move(allocator), TensorBufferTypeMapper(max_small_subspaces_type_id, &_ops)) { } @@ -60,9 +59,10 @@ TensorBufferStore::store_tensor(const Value &tensor) { uint32_t num_subspaces = tensor.index().size(); auto array_size = _ops.get_array_size(num_subspaces); - _add_buffer.resize(array_size); - _ops.store_tensor(_add_buffer, tensor); - return _array_store.add(_add_buffer); + auto ref = _array_store.allocate(array_size); + auto buf = _array_store.get_writable(ref); + _ops.store_tensor(buf, tensor); + return ref; } EntryRef diff --git a/searchlib/src/vespa/searchlib/tensor/tensor_buffer_store.h b/searchlib/src/vespa/searchlib/tensor/tensor_buffer_store.h index 14572eb07dc..18b98efa8fa 100644 --- a/searchlib/src/vespa/searchlib/tensor/tensor_buffer_store.h +++ b/searchlib/src/vespa/searchlib/tensor/tensor_buffer_store.h @@ -23,7 +23,6 @@ class TensorBufferStore : public TensorStore vespalib::eval::ValueType _tensor_type; TensorBufferOperations _ops; ArrayStoreType _array_store; - std::vector<char> _add_buffer; public: TensorBufferStore(const vespalib::eval::ValueType& tensor_type, std::shared_ptr<vespalib::alloc::MemoryAllocator> allocator, uint32_t max_small_subspaces_type_id); ~TensorBufferStore(); diff --git a/searchlib/src/vespa/searchlib/test/fakedata/fakememtreeocc.cpp b/searchlib/src/vespa/searchlib/test/fakedata/fakememtreeocc.cpp index 433f543ab92..11b6a1e3020 100644 --- a/searchlib/src/vespa/searchlib/test/fakedata/fakememtreeocc.cpp +++ b/searchlib/src/vespa/searchlib/test/fakedata/fakememtreeocc.cpp @@ -199,14 +199,14 @@ FakeMemTreeOccMgr::sync() void FakeMemTreeOccMgr::add(uint32_t wordIdx, index::DocIdAndFeatures &features) { - typedef FeatureStore::RefType RefType; - const FakeWord *fw = _fakeWords[wordIdx]; std::pair<EntryRef, uint64_t> r = _featureStore.addFeatures(fw->getPackedIndex(), features); + size_t feature_size = (r.second + 7) / 8; + feature_size += FeatureStore::calc_pad(feature_size); - _featureSizes[wordIdx] += RefType::align((r.second + 7) / 8) * 8; + _featureSizes[wordIdx] += feature_size * 8; _unflushed.push_back(PendingOp(wordIdx, features.doc_id(), r.first)); @@ -240,7 +240,6 @@ FakeMemTreeOccMgr::sortUnflushed() void FakeMemTreeOccMgr::flush() { - typedef FeatureStore::RefType RefType; typedef std::vector<PendingOp>::iterator I; if (_unflushed.empty()) @@ -264,7 +263,9 @@ FakeMemTreeOccMgr::flush() if (i->getRemove()) { if (itr.valid() && itr.getKey() == docId) { uint64_t bits = _featureStore.bitSize(fw->getPackedIndex(), EntryRef(itr.getData().get_features_relaxed())); - _featureSizes[wordIdx] -= RefType::align((bits + 7) / 8) * 8; + size_t feature_size = (bits + 7) / 8; + feature_size += FeatureStore::calc_pad(feature_size); + _featureSizes[wordIdx] -= feature_size * 8; tree.remove(itr); } } else { diff --git a/vespalib/src/tests/datastore/array_store/array_store_test.cpp b/vespalib/src/tests/datastore/array_store/array_store_test.cpp index 74bbf59625b..2ff2897461b 100644 --- a/vespalib/src/tests/datastore/array_store/array_store_test.cpp +++ b/vespalib/src/tests/datastore/array_store/array_store_test.cpp @@ -29,8 +29,8 @@ constexpr float ALLOC_GROW_FACTOR = 0.2; } -template <typename EntryT, typename RefT = EntryRefT<19> > -struct ArrayStoreTest : public testing::Test +template <typename TestT, typename EntryT, typename RefT = EntryRefT<19> > +struct ArrayStoreTest : public TestT { using EntryRefType = RefT; using ArrayStoreType = ArrayStore<EntryT, RefT>; @@ -44,25 +44,39 @@ struct ArrayStoreTest : public testing::Test ArrayStoreType store; ReferenceStore refStore; generation_t generation; - ArrayStoreTest(uint32_t maxSmallArraySize = 3, bool enable_free_lists = true) + bool add_using_allocate; + ArrayStoreTest(uint32_t maxSmallArraySize = 3, bool enable_free_lists = true, bool add_using_allocate_in = false) : store(ArrayStoreConfig(maxSmallArraySize, ArrayStoreConfig::AllocSpec(16, RefT::offsetSize(), 8_Ki, ALLOC_GROW_FACTOR)).enable_free_lists(enable_free_lists), std::make_unique<MemoryAllocatorObserver>(stats)), refStore(), - generation(1) + generation(1), + add_using_allocate(add_using_allocate_in) {} ArrayStoreTest(const ArrayStoreConfig &storeCfg) : store(storeCfg, std::make_unique<MemoryAllocatorObserver>(stats)), refStore(), - generation(1) + generation(1), + add_using_allocate(false) {} void assertAdd(const EntryVector &input) { EntryRef ref = add(input); assertGet(ref, input); } EntryRef add(const EntryVector &input) { - EntryRef result = store.add(ConstArrayRef(input)); + EntryRef result; + if (add_using_allocate) { + result = store.allocate(input.size()); + auto dest = store.get_writable(result); + assert(dest.size() == input.size()); + for (size_t i = 0; i < input.size(); ++i) { + dest[i] = input[i]; + } + } else { + // This is default and preferred way of adding an array. + result = store.add(ConstArrayRef(input)); + } assert(refStore.count(result) == 0); refStore.insert(std::make_pair(result, input)); return result; @@ -148,21 +162,48 @@ struct ArrayStoreTest : public testing::Test size_t largeArraySize() const { return sizeof(LargeArray); } }; -using NumberStoreTest = ArrayStoreTest<uint32_t>; -using StringStoreTest = ArrayStoreTest<std::string>; -using SmallOffsetNumberStoreTest = ArrayStoreTest<uint32_t, EntryRefT<10>>; +struct TestParam { + bool add_using_allocate; + TestParam(bool add_using_allocate_in) : add_using_allocate(add_using_allocate_in) {} +}; + +std::ostream& operator<<(std::ostream& os, const TestParam& param) +{ + os << (param.add_using_allocate ? "add_using_allocate" : "basic_add"); + return os; +} + +using NumberStoreTestWithParam = ArrayStoreTest<testing::TestWithParam<TestParam>, uint32_t>; + +struct NumberStoreTest : public NumberStoreTestWithParam { + NumberStoreTest() : NumberStoreTestWithParam(3, true, GetParam().add_using_allocate) {} +}; -struct NumberStoreFreeListsDisabledTest : public NumberStoreTest { - NumberStoreFreeListsDisabledTest() : NumberStoreTest(3, false) {} +struct NumberStoreFreeListsDisabledTest : public NumberStoreTestWithParam { + NumberStoreFreeListsDisabledTest() : NumberStoreTestWithParam(3, false, GetParam().add_using_allocate) {} }; +using NumberStoreBasicTest = ArrayStoreTest<testing::Test, uint32_t>; +using StringStoreTest = ArrayStoreTest<testing::Test, std::string>; +using SmallOffsetNumberStoreTest = ArrayStoreTest<testing::Test, uint32_t, EntryRefT<10>>; + TEST(BasicStoreTest, test_with_trivial_and_non_trivial_types) { - EXPECT_TRUE(vespalib::can_skip_destruction<NumberStoreTest::value_type>); + EXPECT_TRUE(vespalib::can_skip_destruction<NumberStoreBasicTest::value_type>); EXPECT_FALSE(vespalib::can_skip_destruction<StringStoreTest::value_type>); } -TEST_F(NumberStoreTest, control_static_sizes) { +VESPA_GTEST_INSTANTIATE_TEST_SUITE_P(NumberStoreMultiTest, + NumberStoreTest, + testing::Values(TestParam(false), TestParam(true)), + testing::PrintToStringParamName()); + +VESPA_GTEST_INSTANTIATE_TEST_SUITE_P(NumberStoreFreeListsDisabledMultiTest, + NumberStoreFreeListsDisabledTest, + testing::Values(TestParam(false), TestParam(true)), + testing::PrintToStringParamName()); + +TEST_P(NumberStoreTest, control_static_sizes) { #ifdef _LIBCPP_VERSION EXPECT_EQ(440u, sizeof(f.store)); EXPECT_EQ(296u, sizeof(NumberStoreTest::ArrayStoreType::DataStoreType)); @@ -176,7 +217,7 @@ TEST_F(NumberStoreTest, control_static_sizes) { EXPECT_EQ(32u, usage.usedBytes()); } -TEST_F(NumberStoreTest, add_and_get_small_arrays_of_trivial_type) +TEST_P(NumberStoreTest, add_and_get_small_arrays_of_trivial_type) { assertAdd({}); assertAdd({1}); @@ -192,7 +233,7 @@ TEST_F(StringStoreTest, add_and_get_small_arrays_of_non_trivial_type) assertAdd({"ddd", "eeee", "fffff"}); } -TEST_F(NumberStoreTest, add_and_get_large_arrays_of_simple_type) +TEST_P(NumberStoreTest, add_and_get_large_arrays_of_simple_type) { assertAdd({1,2,3,4}); assertAdd({2,3,4,5,6}); @@ -204,7 +245,7 @@ TEST_F(StringStoreTest, add_and_get_large_arrays_of_non_trivial_type) assertAdd({"ddd", "eee", "ffff", "gggg", "hhhh"}); } -TEST_F(NumberStoreTest, elements_are_put_on_hold_when_a_small_array_is_removed) +TEST_P(NumberStoreTest, elements_are_put_on_hold_when_a_small_array_is_removed) { EntryRef ref = add({1,2,3}); assertBufferState(ref, MemStats().used(3).hold(0)); @@ -212,7 +253,7 @@ TEST_F(NumberStoreTest, elements_are_put_on_hold_when_a_small_array_is_removed) assertBufferState(ref, MemStats().used(3).hold(3)); } -TEST_F(NumberStoreTest, elements_are_put_on_hold_when_a_large_array_is_removed) +TEST_P(NumberStoreTest, elements_are_put_on_hold_when_a_large_array_is_removed) { EntryRef ref = add({1,2,3,4}); // Note: The first buffer has the first element reserved -> we expect 2 elements used here. @@ -221,23 +262,23 @@ TEST_F(NumberStoreTest, elements_are_put_on_hold_when_a_large_array_is_removed) assertBufferState(ref, MemStats().used(2).hold(1).dead(1)); } -TEST_F(NumberStoreTest, small_arrays_are_allocated_from_free_lists_when_enabled) { +TEST_P(NumberStoreTest, small_arrays_are_allocated_from_free_lists_when_enabled) { assert_ref_reused({1,2,3}, {4,5,6}, true); } -TEST_F(NumberStoreTest, large_arrays_are_allocated_from_free_lists_when_enabled) { +TEST_P(NumberStoreTest, large_arrays_are_allocated_from_free_lists_when_enabled) { assert_ref_reused({1,2,3,4}, {5,6,7,8}, true); } -TEST_F(NumberStoreFreeListsDisabledTest, small_arrays_are_NOT_allocated_from_free_lists_when_disabled) { +TEST_P(NumberStoreFreeListsDisabledTest, small_arrays_are_NOT_allocated_from_free_lists_when_disabled) { assert_ref_reused({1,2,3}, {4,5,6}, false); } -TEST_F(NumberStoreFreeListsDisabledTest, large_arrays_are_NOT_allocated_from_free_lists_when_disabled) { +TEST_P(NumberStoreFreeListsDisabledTest, large_arrays_are_NOT_allocated_from_free_lists_when_disabled) { assert_ref_reused({1,2,3,4}, {5,6,7,8}, false); } -TEST_F(NumberStoreTest, track_size_of_large_array_allocations_with_free_lists_enabled) { +TEST_P(NumberStoreTest, track_size_of_large_array_allocations_with_free_lists_enabled) { EntryRef ref = add({1,2,3,4}); assert_buffer_stats(ref, BufferStats().used(2).hold(0).dead(1).extra_used(16)); remove({1,2,3,4}); @@ -269,7 +310,7 @@ TEST_F(SmallOffsetNumberStoreTest, new_underlying_buffer_is_allocated_when_curre namespace { void -test_compaction(NumberStoreTest &f) +test_compaction(NumberStoreBasicTest &f) { EntryRef size1Ref = f.add({1}); EntryRef size2Ref = f.add({2,2}); @@ -300,8 +341,8 @@ test_compaction(NumberStoreTest &f) } -struct NumberStoreTwoSmallBufferTypesTest : public NumberStoreTest { - NumberStoreTwoSmallBufferTypesTest() : NumberStoreTest(2) {} +struct NumberStoreTwoSmallBufferTypesTest : public NumberStoreBasicTest { + NumberStoreTwoSmallBufferTypesTest() : NumberStoreBasicTest(2) {} }; TEST_F(NumberStoreTwoSmallBufferTypesTest, buffer_with_most_dead_space_is_compacted) @@ -372,23 +413,23 @@ void testCompaction(NumberStoreTest &f, bool compactMemory, bool compactAddressS } -TEST_F(NumberStoreTest, compactWorst_selects_on_only_memory) { +TEST_P(NumberStoreTest, compactWorst_selects_on_only_memory) { testCompaction(*this, true, false); } -TEST_F(NumberStoreTest, compactWorst_selects_on_only_address_space) { +TEST_P(NumberStoreTest, compactWorst_selects_on_only_address_space) { testCompaction(*this, false, true); } -TEST_F(NumberStoreTest, compactWorst_selects_on_both_memory_and_address_space) { +TEST_P(NumberStoreTest, compactWorst_selects_on_both_memory_and_address_space) { testCompaction(*this, true, true); } -TEST_F(NumberStoreTest, compactWorst_selects_on_neither_memory_nor_address_space) { +TEST_P(NumberStoreTest, compactWorst_selects_on_neither_memory_nor_address_space) { testCompaction(*this, false, false); } -TEST_F(NumberStoreTest, used_onHold_and_dead_memory_usage_is_tracked_for_small_arrays) +TEST_P(NumberStoreTest, used_onHold_and_dead_memory_usage_is_tracked_for_small_arrays) { MemStats exp(store.getMemoryUsage()); add({1,2,3}); @@ -399,7 +440,7 @@ TEST_F(NumberStoreTest, used_onHold_and_dead_memory_usage_is_tracked_for_small_a assertMemoryUsage(exp.holdToDead(entrySize() * 3)); } -TEST_F(NumberStoreTest, used_onHold_and_dead_memory_usage_is_tracked_for_large_arrays) +TEST_P(NumberStoreTest, used_onHold_and_dead_memory_usage_is_tracked_for_large_arrays) { MemStats exp(store.getMemoryUsage()); add({1,2,3,4}); @@ -411,7 +452,7 @@ TEST_F(NumberStoreTest, used_onHold_and_dead_memory_usage_is_tracked_for_large_a dead(largeArraySize())); } -TEST_F(NumberStoreTest, address_space_usage_is_ratio_between_used_arrays_and_number_of_possible_arrays) +TEST_P(NumberStoreTest, address_space_usage_is_ratio_between_used_arrays_and_number_of_possible_arrays) { add({2,2}); add({3,3,3}); @@ -435,8 +476,8 @@ TEST_F(NumberStoreTest, address_space_usage_is_ratio_between_used_arrays_and_num EXPECT_EQ(expLimit, store.addressSpaceUsage().limit()); } -struct ByteStoreTest : public ArrayStoreTest<uint8_t> { - ByteStoreTest() : ArrayStoreTest<uint8_t>(ByteStoreTest::ArrayStoreType:: +struct ByteStoreTest : public ArrayStoreTest<testing::Test, uint8_t> { + ByteStoreTest() : ArrayStoreTest<testing::Test, uint8_t>(ByteStoreTest::ArrayStoreType:: optimizedConfigForHugePage(1023, vespalib::alloc::MemoryAllocator::HUGEPAGE_SIZE, 4_Ki, 8_Ki, ALLOC_GROW_FACTOR)) {} @@ -452,7 +493,7 @@ TEST_F(ByteStoreTest, offset_in_EntryRefT_is_within_bounds_when_allocating_memor assertStoreContent(); } -TEST_F(NumberStoreTest, provided_memory_allocator_is_used) +TEST_P(NumberStoreTest, provided_memory_allocator_is_used) { EXPECT_EQ(AllocStats(4, 0), stats); } diff --git a/vespalib/src/tests/datastore/datastore/datastore_test.cpp b/vespalib/src/tests/datastore/datastore/datastore_test.cpp index 964978e5510..b77599c4e34 100644 --- a/vespalib/src/tests/datastore/datastore/datastore_test.cpp +++ b/vespalib/src/tests/datastore/datastore/datastore_test.cpp @@ -215,34 +215,6 @@ TEST(DataStoreTest, require_that_entry_ref_is_working) } } -TEST(DataStoreTest, require_that_aligned_entry_ref_is_working) -{ - using MyRefType = AlignedEntryRefT<22, 2>; // 4 byte alignement - EXPECT_EQ(16_Mi, MyRefType::offsetSize()); - EXPECT_EQ(1_Ki, MyRefType::numBuffers()); - EXPECT_EQ(0u, MyRefType::align(0)); - EXPECT_EQ(4u, MyRefType::align(1)); - EXPECT_EQ(4u, MyRefType::align(2)); - EXPECT_EQ(4u, MyRefType::align(3)); - EXPECT_EQ(4u, MyRefType::align(4)); - EXPECT_EQ(8u, MyRefType::align(5)); - { - MyRefType r(0, 0); - EXPECT_EQ(0u, r.offset()); - EXPECT_EQ(0u, r.bufferId()); - } - { - MyRefType r(237, 13); - EXPECT_EQ(MyRefType::align(237), r.offset()); - EXPECT_EQ(13u, r.bufferId()); - } - { - MyRefType r(MyRefType::offsetSize() - 4, 1023); - EXPECT_EQ(MyRefType::align(MyRefType::offsetSize() - 4), r.offset()); - EXPECT_EQ(1023u, r.bufferId()); - } -} - TEST(DataStoreTest, require_that_entries_can_be_added_and_retrieved) { using IntStore = DataStore<int>; diff --git a/vespalib/src/vespa/vespalib/datastore/array_store.h b/vespalib/src/vespa/vespalib/datastore/array_store.h index 2c6aaea6b61..db037ee12fb 100644 --- a/vespalib/src/vespa/vespalib/datastore/array_store.h +++ b/vespalib/src/vespa/vespalib/datastore/array_store.h @@ -51,7 +51,9 @@ private: void initArrayTypes(const ArrayStoreConfig &cfg, std::shared_ptr<alloc::MemoryAllocator> memory_allocator); EntryRef addSmallArray(const ConstArrayRef &array); + EntryRef allocate_small_array(size_t array_size); EntryRef addLargeArray(const ConstArrayRef &array); + EntryRef allocate_large_array(size_t array_size); ConstArrayRef getSmallArray(RefT ref, size_t arraySize) const { const EntryT *buf = _store.template getEntryArray<EntryT>(ref, arraySize); return ConstArrayRef(buf, arraySize); @@ -81,6 +83,17 @@ public: } /** + * Allocate an array of the given size without any instantiation of EntryT elements. + * + * Use get_writable() to get a reference to the array for writing. + * + * NOTE: In most cases add() should be used instead. + * This function is however relevant when serializing objects into char buffers + * when e.g. using an ArrayStore<char> for memory management. + */ + EntryRef allocate(size_t array_size); + + /** * Returns a writeable reference to the given array. * * NOTE: Use with care if reader threads are accessing arrays at the same time. diff --git a/vespalib/src/vespa/vespalib/datastore/array_store.hpp b/vespalib/src/vespa/vespalib/datastore/array_store.hpp index 308afd2b122..4fc13396f6b 100644 --- a/vespalib/src/vespa/vespalib/datastore/array_store.hpp +++ b/vespalib/src/vespa/vespalib/datastore/array_store.hpp @@ -76,6 +76,20 @@ ArrayStore<EntryT, RefT, TypeMapperT>::add(const ConstArrayRef &array) template <typename EntryT, typename RefT, typename TypeMapperT> EntryRef +ArrayStore<EntryT, RefT, TypeMapperT>::allocate(size_t array_size) +{ + if (array_size == 0) { + return EntryRef(); + } + if (array_size <= _maxSmallArraySize) { + return allocate_small_array(array_size); + } else { + return allocate_large_array(array_size); + } +} + +template <typename EntryT, typename RefT, typename TypeMapperT> +EntryRef ArrayStore<EntryT, RefT, TypeMapperT>::addSmallArray(const ConstArrayRef &array) { uint32_t typeId = _mapper.get_type_id(array.size()); @@ -85,6 +99,14 @@ ArrayStore<EntryT, RefT, TypeMapperT>::addSmallArray(const ConstArrayRef &array) template <typename EntryT, typename RefT, typename TypeMapperT> EntryRef +ArrayStore<EntryT, RefT, TypeMapperT>::allocate_small_array(size_t array_size) +{ + uint32_t type_id = _mapper.get_type_id(array_size); + return _store.template freeListRawAllocator<EntryT>(type_id).alloc(array_size).ref; +} + +template <typename EntryT, typename RefT, typename TypeMapperT> +EntryRef ArrayStore<EntryT, RefT, TypeMapperT>::addLargeArray(const ConstArrayRef &array) { using NoOpReclaimer = DefaultReclaimer<LargeArray>; @@ -96,6 +118,17 @@ ArrayStore<EntryT, RefT, TypeMapperT>::addLargeArray(const ConstArrayRef &array) } template <typename EntryT, typename RefT, typename TypeMapperT> +EntryRef +ArrayStore<EntryT, RefT, TypeMapperT>::allocate_large_array(size_t array_size) +{ + using NoOpReclaimer = DefaultReclaimer<LargeArray>; + auto handle = _store.template freeListAllocator<LargeArray, NoOpReclaimer>(_largeArrayTypeId).alloc(array_size); + auto& state = _store.getBufferState(RefT(handle.ref).bufferId()); + state.incExtraUsedBytes(sizeof(EntryT) * array_size); + return handle.ref; +} + +template <typename EntryT, typename RefT, typename TypeMapperT> void ArrayStore<EntryT, RefT, TypeMapperT>::remove(EntryRef ref) { diff --git a/vespalib/src/vespa/vespalib/datastore/datastore.hpp b/vespalib/src/vespa/vespalib/datastore/datastore.hpp index 666da55975c..4d09ffe4bc6 100644 --- a/vespalib/src/vespa/vespalib/datastore/datastore.hpp +++ b/vespalib/src/vespa/vespalib/datastore/datastore.hpp @@ -13,8 +13,7 @@ namespace vespalib::datastore { template <typename RefT> DataStoreT<RefT>::DataStoreT() - : DataStoreBase(RefType::numBuffers(), - RefType::unscaled_offset_size()) + : DataStoreBase(RefType::numBuffers(), RefType::offsetSize()) { } @@ -42,7 +41,7 @@ DataStoreT<RefT>::free_elem_internal(EntryRef ref, size_t numElems, bool was_hel state.decHoldElems(numElems); } state.cleanHold(getBuffer(intRef.bufferId()), - intRef.unscaled_offset() * state.getArraySize(), numElems); + intRef.offset() * state.getArraySize(), numElems); } template <typename RefT> @@ -50,15 +49,14 @@ void DataStoreT<RefT>::holdElem(EntryRef ref, size_t numElems, size_t extraBytes) { RefType intRef(ref); - size_t alignedLen = RefType::align(numElems); BufferState &state = getBufferState(intRef.bufferId()); assert(state.isActive()); if (state.hasDisabledElemHoldList()) { - state.incDeadElems(alignedLen); + state.incDeadElems(numElems); return; } - _elemHold1List.push_back(ElemHold1ListElem(ref, alignedLen)); - state.incHoldElems(alignedLen); + _elemHold1List.push_back(ElemHold1ListElem(ref, numElems)); + state.incHoldElems(numElems); state.incExtraHoldBytes(extraBytes); } diff --git a/vespalib/src/vespa/vespalib/datastore/entryref.h b/vespalib/src/vespa/vespalib/datastore/entryref.h index 7667cc3d2c1..a0016f4fdcb 100644 --- a/vespalib/src/vespa/vespalib/datastore/entryref.h +++ b/vespalib/src/vespa/vespalib/datastore/entryref.h @@ -40,34 +40,6 @@ public: uint32_t bufferId() const noexcept { return _ref >> OffsetBits; } static size_t offsetSize() noexcept { return 1ul << OffsetBits; } static uint32_t numBuffers() noexcept { return 1 << BufferBits; } - static size_t align(size_t val) noexcept { return val; } - static size_t pad(size_t val) noexcept { (void) val; return 0ul; } - static constexpr bool isAlignedType = false; - // TODO: Remove following temporary methods when removing - // AlignedEntryRefT - size_t unscaled_offset() const noexcept { return offset(); } - static size_t unscaled_offset_size() noexcept { return offsetSize(); } -}; - -/** - * Class for entry reference that is similar to EntryRefT, - * except that we use (2^OffsetAlign) byte alignment on the offset. - **/ -template <uint32_t OffsetBits, uint32_t OffsetAlign> -class AlignedEntryRefT : public EntryRefT<OffsetBits> { -private: - typedef EntryRefT<OffsetBits> ParentType; - static const uint32_t PadConstant = ((1 << OffsetAlign) - 1); -public: - AlignedEntryRefT() noexcept : ParentType() {} - AlignedEntryRefT(size_t offset_, uint32_t bufferId_) noexcept : - ParentType(align(offset_) >> OffsetAlign, bufferId_) {} - AlignedEntryRefT(const EntryRef & ref_) noexcept : ParentType(ref_) {} - size_t offset() const { return ParentType::offset() << OffsetAlign; } - static size_t offsetSize() { return ParentType::offsetSize() << OffsetAlign; } - static size_t align(size_t val) { return val + pad(val); } - static size_t pad(size_t val) { return (-val & PadConstant); } - static constexpr bool isAlignedType = true; }; vespalib::asciistream& operator<<(vespalib::asciistream& os, const EntryRef& ref); diff --git a/vespalib/src/vespa/vespalib/datastore/free_list_raw_allocator.hpp b/vespalib/src/vespa/vespalib/datastore/free_list_raw_allocator.hpp index 55d57b1bcb9..c4689cd9e4a 100644 --- a/vespalib/src/vespa/vespalib/datastore/free_list_raw_allocator.hpp +++ b/vespalib/src/vespa/vespalib/datastore/free_list_raw_allocator.hpp @@ -24,10 +24,8 @@ FreeListRawAllocator<EntryT, RefT>::alloc(size_t numElems) assert(state.isActive()); assert(state.getArraySize() == numElems); RefT ref = state.popFreeList(); - // If entry ref is not aligned we must scale the offset according to array size as it was divided when the entry ref was created. - EntryT *entry = !RefT::isAlignedType ? - _store.template getEntryArray<EntryT>(ref, state.getArraySize()) : - _store.template getEntry<EntryT>(ref); + // We must scale the offset according to array size as it was divided when the entry ref was created. + EntryT *entry = _store.template getEntryArray<EntryT>(ref, state.getArraySize()); return HandleType(ref, entry); } diff --git a/vespalib/src/vespa/vespalib/datastore/raw_allocator.hpp b/vespalib/src/vespa/vespalib/datastore/raw_allocator.hpp index a17c3a28ced..0d67bf71c20 100644 --- a/vespalib/src/vespa/vespalib/datastore/raw_allocator.hpp +++ b/vespalib/src/vespa/vespalib/datastore/raw_allocator.hpp @@ -23,21 +23,13 @@ RawAllocator<EntryT, RefT>::alloc(size_t numElems, size_t extraElems) BufferState &state = _store.getBufferState(buffer_id); assert(state.isActive()); size_t oldBufferSize = state.size(); - if (RefT::isAlignedType) { - // AlignedEntryRef constructor scales down offset by alignment - RefT ref(oldBufferSize, buffer_id); - EntryT *buffer = _store.getEntry<EntryT>(ref); - state.pushed_back(numElems); - return HandleType(ref, buffer); - } else { - // Must perform scaling ourselves, according to array size - size_t arraySize = state.getArraySize(); - assert((numElems % arraySize) == 0u); - RefT ref((oldBufferSize / arraySize), buffer_id); - EntryT *buffer = _store.getEntryArray<EntryT>(ref, arraySize); - state.pushed_back(numElems); - return HandleType(ref, buffer); - } + // Must perform scaling ourselves, according to array size + size_t arraySize = state.getArraySize(); + assert((numElems % arraySize) == 0u); + RefT ref((oldBufferSize / arraySize), buffer_id); + EntryT *buffer = _store.getEntryArray<EntryT>(ref, arraySize); + state.pushed_back(numElems); + return HandleType(ref, buffer); } } diff --git a/vespalib/src/vespa/vespalib/datastore/unique_store.hpp b/vespalib/src/vespa/vespalib/datastore/unique_store.hpp index a252763fb5b..cbb8369e1f2 100644 --- a/vespalib/src/vespa/vespalib/datastore/unique_store.hpp +++ b/vespalib/src/vespa/vespalib/datastore/unique_store.hpp @@ -113,8 +113,8 @@ private: RefT iRef(oldRef); uint32_t buffer_id = iRef.bufferId(); auto &inner_mapping = _mapping[buffer_id]; - assert(iRef.unscaled_offset() < inner_mapping.size()); - EntryRef &mappedRef = inner_mapping[iRef.unscaled_offset()]; + assert(iRef.offset() < inner_mapping.size()); + EntryRef &mappedRef = inner_mapping[iRef.offset()]; assert(!mappedRef.valid()); EntryRef newRef = _store.move(oldRef); mappedRef = newRef; diff --git a/vespalib/src/vespa/vespalib/datastore/unique_store_enumerator.h b/vespalib/src/vespa/vespalib/datastore/unique_store_enumerator.h index e6627eb80e6..c4baff2206b 100644 --- a/vespalib/src/vespa/vespalib/datastore/unique_store_enumerator.h +++ b/vespalib/src/vespa/vespalib/datastore/unique_store_enumerator.h @@ -47,8 +47,8 @@ public: uint32_t mapEntryRefToEnumValue(EntryRef ref) const { if (ref.valid()) { RefType iRef(ref); - assert(iRef.unscaled_offset() < _enumValues[iRef.bufferId()].size()); - uint32_t enumValue = _enumValues[iRef.bufferId()][iRef.unscaled_offset()]; + assert(iRef.offset() < _enumValues[iRef.bufferId()].size()); + uint32_t enumValue = _enumValues[iRef.bufferId()][iRef.offset()]; assert(enumValue != 0); return enumValue; } else { @@ -59,8 +59,8 @@ public: uint32_t map_entry_ref_to_enum_value_or_zero(EntryRef ref) const { if (ref.valid()) { RefType iRef(ref); - if (iRef.unscaled_offset() < _enumValues[iRef.bufferId()].size()) { - return _enumValues[iRef.bufferId()][iRef.unscaled_offset()]; + if (iRef.offset() < _enumValues[iRef.bufferId()].size()) { + return _enumValues[iRef.bufferId()][iRef.offset()]; } else { return 0u; } diff --git a/vespalib/src/vespa/vespalib/datastore/unique_store_enumerator.hpp b/vespalib/src/vespa/vespalib/datastore/unique_store_enumerator.hpp index 0cc6b4eded2..52437fc765c 100644 --- a/vespalib/src/vespa/vespalib/datastore/unique_store_enumerator.hpp +++ b/vespalib/src/vespa/vespalib/datastore/unique_store_enumerator.hpp @@ -31,8 +31,8 @@ UniqueStoreEnumerator<RefT>::enumerateValue(EntryRef ref) { RefType iRef(ref); assert(iRef.valid()); - assert(iRef.unscaled_offset() < _enumValues[iRef.bufferId()].size()); - uint32_t &enumVal = _enumValues[iRef.bufferId()][iRef.unscaled_offset()]; + assert(iRef.offset() < _enumValues[iRef.bufferId()].size()); + uint32_t &enumVal = _enumValues[iRef.bufferId()][iRef.offset()]; assert(enumVal == 0u); enumVal = _next_enum_val; ++_next_enum_val; diff --git a/vespalib/src/vespa/vespalib/datastore/unique_store_remapper.h b/vespalib/src/vespa/vespalib/datastore/unique_store_remapper.h index 80486f55ad8..4babd6204c7 100644 --- a/vespalib/src/vespa/vespalib/datastore/unique_store_remapper.h +++ b/vespalib/src/vespa/vespalib/datastore/unique_store_remapper.h @@ -32,8 +32,8 @@ public: EntryRef remap(EntryRef ref) const { RefType internal_ref(ref); auto &inner_mapping = _mapping[internal_ref.bufferId()]; - assert(internal_ref.unscaled_offset() < inner_mapping.size()); - EntryRef mapped_ref = inner_mapping[internal_ref.unscaled_offset()]; + assert(internal_ref.offset() < inner_mapping.size()); + EntryRef mapped_ref = inner_mapping[internal_ref.offset()]; assert(mapped_ref.valid()); return mapped_ref; } |