Repository: MystenLabs/sui
File: external-crates/move/crates/move-vm-runtime/src/cache/move_cache.rs (Rust)

Code before:

    // -------------------------------------------
    // Package Caching Operations
    // -------------------------------------------

    /// Add a package to the cache. If the package is already present, this is a no-op.
    ///
    /// Important: it is not an error if a package is already present when we go to insert a
    /// package. This can happen in concurrent scenarios where multiple threads attempt to load the
    /// same package at the same time -- they could both check that the package is not yet cached
    /// with `cached_package_at`, and then both proceed to independenctly load and verify the
    /// package. As the write lock is not held between the `cached_package_at` call and the call to
    /// `add_to_cache` the package could be inserted by another thread in the meantime.
    ///
    /// Returns `true` if the package was newly inserted, `false` if it was already present.
    /// NB: in a parallel scenario the result of this function is not guaranteed to be
    /// deterministic.
    pub(crate) fn add_package_to_cache(
        &self,
        package_key: VersionId,
        verified: verification::ast::Package,
        runtime: jit::execution::ast::Package,
    ) -> bool {
        // Check if the package is already present, and if so, return false early to avoid
        // re-inserting the already-cached package.
        if self.package_cache.contains_key(&package_key) {
            return false;
        }
        let verified = Arc::new(verified);
        let runtime = Arc::new(runtime);
        let package = Package { verified, runtime };
        self.package_cache
            .entry(package_key)
            .or_insert(Arc::new(package));
        true
    }

    /// Get a package from the cache, if it is present.
    /// If not present, returns `None`.
    pub(crate) fn cached_package_at(&self, package_key: VersionId) -> Option<Arc<Package>> {
        self.package_cache
            .get(&package_key)
            .as_deref()
            .map(Arc::clone)
    }

    // -------------------------------------------
    // Linkage VTable Caching Operations
    // -------------------------------------------

    /// Add linkage tables to the cache for a given linkage context.

Review comment (bug):
Sorry to be so nitty on this function...

The return value of this function is incorrect now, even with the `entry` since if it gets cached between the check and insertion, it could not be present on this check, but then at line 100 the `entry` would be filled and we wouldn't insert the new package, but nevertheless return `true`. 

We only use this boolean to track cache loads I think, so it's not a big deal, but might be worth either calling out in a comment, or removing the `bool` return entirely. 

Code after:

    // -------------------------------------------
    // Package Caching Operations
    // -------------------------------------------

    /// Add a package to the cache. If the package is already present, this is a no-op.
    ///
    /// Important: it is not an error if a package is already present when we go to insert a
    /// package. This can happen in concurrent scenarios where multiple threads attempt to load the
    /// same package at the same time -- they could both check that the package is not yet cached
    /// with `cached_package_at`, and then both proceed to independenctly load and verify the
    /// package. As the write lock is not held between the `cached_package_at` call and the call to
    /// `add_to_cache` the package could be inserted by another thread in the meantime.
    ///
    /// Returns `true` if the package was newly inserted, `false` if it was already present.
    /// NB: in a parallel scenario the result of this function is not guaranteed to be
    /// deterministic.
    pub(crate) fn add_package_to_cache(
        &self,
        package_key: VersionId,
        verified: verification::ast::Package,
        runtime: jit::execution::ast::Package,
    ) -> bool {
        use dashmap::mapref::entry::Entry;
        // Grab the entry at the top, so we can figure out which flag to return while holding the
        // lock on the shard of dashmap we are modifying (so this does not change out from under us
        // mid-write).
        let entry = self.package_cache.entry(package_key);
        match entry {
            Entry::Occupied(_) => {
                // Package is already present.
                false
            }
            Entry::Vacant(vacant_entry) => {
                let verified = Arc::new(verified);
                let runtime = Arc::new(runtime);
                let package = Package { verified, runtime };
                vacant_entry.insert(Arc::new(package));
                true
            }
        }
    }

    /// Get a package from the cache, if it is present.
    /// If not present, returns `None`.
    pub(crate) fn cached_package_at(&self, package_key: VersionId) -> Option<Arc<Package>> {
        self.package_cache
            .get(&package_key)
            .as_deref()
            .map(Arc::clone)
    }