Make Cache.get() just return the value. Change-Id: I816a1c28569011215ad6cba9a7559513a7900386 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/246880 Commit-Queue: Konstantin Shcheglov <scheglov@google.com> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
diff --git a/pkg/analyzer/lib/src/dart/analysis/byte_store.dart b/pkg/analyzer/lib/src/dart/analysis/byte_store.dart index 7fa38f7..0c096b3 100644 --- a/pkg/analyzer/lib/src/dart/analysis/byte_store.dart +++ b/pkg/analyzer/lib/src/dart/analysis/byte_store.dart
@@ -50,7 +50,18 @@ @override Uint8List? get(String key) { - return _cache.get(key, () => _store.get(key)); + final cached = _cache.get(key); + if (cached != null) { + return cached; + } + + final fromStore = _store.get(key); + if (fromStore != null) { + _cache.put(key, fromStore); + return fromStore; + } + + return null; } @override
diff --git a/pkg/analyzer/lib/src/dart/analysis/cache.dart b/pkg/analyzer/lib/src/dart/analysis/cache.dart index ca97db6..bddb6fa 100644 --- a/pkg/analyzer/lib/src/dart/analysis/cache.dart +++ b/pkg/analyzer/lib/src/dart/analysis/cache.dart
@@ -2,37 +2,33 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. +import 'package:meta/meta.dart'; + /// LRU cache of objects. class Cache<K, V> { final int _maxSizeBytes; final int Function(V) _meter; - final _map = <K, V>{}; + @visibleForTesting + final map = <K, V>{}; int _currentSizeBytes = 0; Cache(this._maxSizeBytes, this._meter); - V? get(K key, V? Function() getNotCached) { - V? value = _map.remove(key); - if (value == null) { - value = getNotCached(); - if (value != null) { - _map[key] = value; - _currentSizeBytes += _meter(value); - _evict(); - } - } else { - _map[key] = value; + V? get(K key) { + final value = map.remove(key); + if (value != null) { + map[key] = value; } return value; } void put(K key, V value) { - V? oldValue = _map[key]; + V? oldValue = map[key]; if (oldValue != null) { _currentSizeBytes -= _meter(oldValue); } - _map[key] = value; + map[key] = value; _currentSizeBytes += _meter(value); _evict(); } @@ -40,7 +36,7 @@ void _evict() { if (_currentSizeBytes > _maxSizeBytes) { var keysToRemove = <K>[]; - for (var entry in _map.entries) { + for (var entry in map.entries) { keysToRemove.add(entry.key); _currentSizeBytes -= _meter(entry.value); if (_currentSizeBytes <= _maxSizeBytes) { @@ -48,7 +44,7 @@ } } for (var key in keysToRemove) { - _map.remove(key); + map.remove(key); } } }
diff --git a/pkg/analyzer/lib/src/dart/micro/cider_byte_store.dart b/pkg/analyzer/lib/src/dart/micro/cider_byte_store.dart index dc78189..0ffa205 100644 --- a/pkg/analyzer/lib/src/dart/micro/cider_byte_store.dart +++ b/pkg/analyzer/lib/src/dart/micro/cider_byte_store.dart
@@ -54,7 +54,7 @@ @override CacheData? get(String key, Uint8List signature) { - var entry = _cache.get(key, () => null); + final entry = _cache.get(key); if (entry != null && const ListEquality<int>().equals(entry.signature, signature)) {
diff --git a/pkg/analyzer/test/src/dart/analysis/cache_test.dart b/pkg/analyzer/test/src/dart/analysis/cache_test.dart index 9a6cac3..0531aa6 100644 --- a/pkg/analyzer/test/src/dart/analysis/cache_test.dart +++ b/pkg/analyzer/test/src/dart/analysis/cache_test.dart
@@ -2,7 +2,10 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. +import 'dart:typed_data'; + import 'package:analyzer/src/dart/analysis/cache.dart'; +import 'package:analyzer_utilities/testing/map_entry_matcher.dart'; import 'package:test/test.dart'; import 'package:test_reflective_loader/test_reflective_loader.dart'; @@ -12,52 +15,47 @@ }); } -List<int> _b(int length) { - return List<int>.filled(length, 0); +Uint8List _b(int length) { + return Uint8List(length); } @reflectiveTest class CacheTest { - test_get_notFound_evict() { - var cache = _newBytesCache(100); + test_get() { + final cache = _newBytesCache(100); - // Request '1'. Nothing found. - expect(cache.get('1', _noBytes), isNull); + expect(cache.get('1'), isNull); + expect(cache.map.entries, isEmpty); - // Add enough data to the store to force an eviction. - cache.put('2', _b(40)); - cache.put('3', _b(40)); - cache.put('4', _b(40)); + cache.put('1', _b(10)); + expect(cache.map.entries, [ + isMapEntry('1', hasLength(10)), + ]); + + expect(cache.get('1'), hasLength(10)); } - test_get_notFound_retry() { - var cache = _newBytesCache(100); + test_get_reorders() { + final cache = _newBytesCache(100); - // Request '1'. Nothing found. - expect(cache.get('1', _noBytes), isNull); + cache.put('1', _b(1)); + cache.put('2', _b(2)); + cache.put('3', _b(3)); + cache.put('4', _b(4)); + expect(cache.map.entries, [ + isMapEntry('1', hasLength(1)), + isMapEntry('2', hasLength(2)), + isMapEntry('3', hasLength(3)), + isMapEntry('4', hasLength(4)), + ]); - // Request '1' again. - // The previous `null` result should not have been cached. - expect(cache.get('1', () => _b(40)), isNotNull); - } - - test_get_put_evict() { - var cache = _newBytesCache(100); - - // Keys: [1, 2]. - cache.put('1', _b(40)); - cache.put('2', _b(50)); - - // Request '1', so now it is the most recently used. - // Keys: [2, 1]. - cache.get('1', _noBytes); - - // 40 + 50 + 30 > 100 - // So, '2' is evicted. - cache.put('3', _b(30)); - expect(cache.get('1', _noBytes), hasLength(40)); - expect(cache.get('2', _noBytes), isNull); - expect(cache.get('3', _noBytes), hasLength(30)); + expect(cache.get('2'), hasLength(2)); + expect(cache.map.entries, [ + isMapEntry('1', hasLength(1)), + isMapEntry('3', hasLength(3)), + isMapEntry('4', hasLength(4)), + isMapEntry('2', hasLength(2)), + ]); } test_put_evict_first() { @@ -66,36 +64,39 @@ // 40 + 50 < 100 cache.put('1', _b(40)); cache.put('2', _b(50)); - expect(cache.get('1', _noBytes), hasLength(40)); - expect(cache.get('2', _noBytes), hasLength(50)); + expect(cache.map.entries, [ + isMapEntry('1', hasLength(40)), + isMapEntry('2', hasLength(50)), + ]); // 40 + 50 + 30 > 100 // So, '1' is evicted. cache.put('3', _b(30)); - expect(cache.get('1', _noBytes), isNull); - expect(cache.get('2', _noBytes), hasLength(50)); - expect(cache.get('3', _noBytes), hasLength(30)); + expect(cache.map.entries, [ + isMapEntry('2', hasLength(50)), + isMapEntry('3', hasLength(30)), + ]); } - test_put_evict_firstAndSecond() { + test_put_evict_firstSecond() { var cache = _newBytesCache(100); // 10 + 80 < 100 cache.put('1', _b(10)); cache.put('2', _b(80)); - expect(cache.get('1', _noBytes), hasLength(10)); - expect(cache.get('2', _noBytes), hasLength(80)); + expect(cache.map.entries, [ + isMapEntry('1', hasLength(10)), + isMapEntry('2', hasLength(80)), + ]); // 10 + 80 + 30 > 100 // So, '1' and '2' are evicted. cache.put('3', _b(30)); - expect(cache.get('1', _noBytes), isNull); - expect(cache.get('2', _noBytes), isNull); - expect(cache.get('3', _noBytes), hasLength(30)); + expect(cache.map.entries, [ + isMapEntry('3', hasLength(30)), + ]); } - Cache<String, List<int>> _newBytesCache(int maxSizeBytes) => - Cache<String, List<int>>(maxSizeBytes, (bytes) => bytes.length); - - static List<int>? _noBytes() => null; + Cache<String, Uint8List> _newBytesCache(int maxSizeBytes) => + Cache<String, Uint8List>(maxSizeBytes, (bytes) => bytes.length); }
diff --git a/pkg/analyzer_utilities/lib/testing/map_entry_matcher.dart b/pkg/analyzer_utilities/lib/testing/map_entry_matcher.dart new file mode 100644 index 0000000..9556ee1 --- /dev/null +++ b/pkg/analyzer_utilities/lib/testing/map_entry_matcher.dart
@@ -0,0 +1,31 @@ +// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'package:test/test.dart'; + +/// Matches a [MapEntry] with matching [MapEntry.key] and [MapEntry.value]. +MapEntryMatcher isMapEntry(key, value) => MapEntryMatcher(key, value); + +class MapEntryMatcher extends Matcher { + final Matcher keyMatcher; + final Matcher valueMatcher; + + MapEntryMatcher(key, value) + : keyMatcher = wrapMatcher(key), + valueMatcher = wrapMatcher(value); + + @override + Description describe(Description description) => description + .add('MapEntry(key: ') + .addDescriptionOf(keyMatcher) + .add(', value: ') + .addDescriptionOf(valueMatcher) + .add(')'); + + @override + bool matches(item, Map matchState) => + item is MapEntry && + keyMatcher.matches(item.key, {}) && + valueMatcher.matches(item.value, {}); +}