Podcast #128: We chat with Kent C Dodds about why he loves React and discuss what life was like in the dark days before Git. Listen now.

Hot answers tagged

28

You say cache I say memory leak. You haven't given much thought to your eviction policy yet and that's 95% of the work in writing a decent cache. Your expired entries are only removed when someone tries to access them. Consider the case when the cache is used poorly - i.e. you end up caching a huge number of things that are only accessed once. Without a ...


18

Short version: it looks like you're just trying to get a per-type ObjectCache. That's a basic singleton pattern, so I'm unsure what you're trying to gain through all this extra cruft in the class. If it were me, I would consider the following code: public static class GlobalTypedObjectCache<T> { public static ObjectCache Cache { get; private set; }...


14

Why skimp on braces? Putting braces around the if-block in generateString() costs you almost nothing — not even an extra line of code. Make it a habit to always include explicit braces so that you never write a disaster like this. (Don't think that it can't happen to you! Accidents often have more than one contributing factor — so don't be a contributing ...


12

generateString It creates 8000000 String instances on the heap and stores their references in the array. Every array slot refers to a different String instance. It uses more memory and GC might use a lot of CPU time. generateString1 It uses only one String instance and every index of the array points to this same instance (this is the instance which is ...


11

I have a "generic" cache object: That's actually pretty non-generic. Especially, Class is a raw type. private static final Map<Class, ICacheable> cacheMap = new HashMap<>(); Moreover, static mutable state is nearly always wrong. Use an instance variable and if you really have to a static instance of Cache. It's also strange to restrict the ...


11

The indentation is inconsistent: the test class has its {} indented the same as the class declaration, but the main class has them indented one level more. It's good that you've included a unit test, but it's only testing half of the functionality. What about the getter? put and get as method names don't follow C# conventions, which would be Put and Get -- ...


10

First, use generics properly. Second, why do you track the cache size if when you need it, you get a hardcoded 99? I will assume that was some sort of mistake. Third, if you really want to annoy most truely professional java developers and course instructors (i.e, not these that write Java code as if they were programming in C#, C++ or Pascal), prefix your ...


10

I tried this on 2 different computers and I'm getting inconsistent results. Sometimes the first method is faster, sometimes the other. It also depends on the number of iterations, with a pretty wild spread. I think the outcome depends on: the JVM implementation and version, especially the default garbage collector settings and behavior the currently active ...


8

You have a static dictionary as backing store which means that all cache instances share it. Not saying that this is wrong and it might be intended but you should be aware of it. Your cache apparently is not thread-safe. You need to protect access to the dictionary with locks or if you are using .NET 4 or later use ConcurrentDictionary. If you make the ...


8

First you should rename your instance members in ActiveConnections class - having methods and variables named getOne(), getTwo(), etc, are horrible because at first you'd expect them to return the int value of one, two, three (e.g., returning 1, 2, 3) but in reality they most likely won't. You should either put a lock on your cache or use a ConcurrentMap or ...


8

Before I get to the main issue, first some smaller notes: A type that is nested in a generic type doesn't need to redeclare the type parameters of the parent, it can use them directly: public class Cache<TKey, TValue> { protected class CacheValue { public CacheValue(TValue value) { LastAccess = DateTime.Now; ...


8

The part of this exercise that concerns me most, and has not been addressed, is that you are not creating 8,000,000 full string instances. You are creating 8,000,000 light-weight wrappers to a single 3-char array. Consider the chain of events when you create a new String("abc") instance: The Java compiler identifies the String constant "abc". It allocates ...


8

No Type Safety Currently your classes provide no type safety: Cache cache = new Cache(); cache.put("my key", Integer.class, 5); // ... String cached = cache.get("my key", String.class); This compiles but fails at runtime with a ClassCastException thrown by CachedValue.getValue. What's The Key? The reason why this can happen is that your class is not ...


8

You can instead use a LinkedHashMap, which lets you get the in access order if you made it with the (int, float, boolean) constructor. Then adding means removing map.entrySet().iterator().next().remove() if it becomes too large. However LinkedHashMap is specially designed to let a subclass decide when to remove the oldest entry using the removeEldestEntry ...


8

My experience with variants on the java.util.concurrent.locks.* classes suggests that their sweet spot in terms of performance is when the work to be done is relatively substantial compared to the lock overhead time. Remember, when you use Reentrant or ReadWrite locks, that there is a a call at both the beginning and end of the locked block. As a ...


8

Instead of doing a whole lot of if-else over the supported constructor classes, you could use Reflection to accomplish the construction of objects. cacheableElementClass.getDeclaredConstructor or getDeclaredConstructors to retrieve the constructor you want to invoke. Constructor.newInstance(params) to create the constructor. Sample code that might work: ...


8

Some nice syntactic tricks in there which I didn't know about, so firstly thanks for that. Aside from that I don't have much to say about the code, but there are a few things about the design which don't convince me. Why one class without type parameters? This has two aspects. Firstly, I would prefer to use one instance per logical cache and eliminate ...


7

Here are a few comments: you should not start a thread from the constructor - for example, the thread could see mPagesStatus as null and throw a NullPointerException. So you should provide an init method for example, that must be called by the client. I'm not a big fan of Hungarian notation (prefixing all instance variables with m) - my IDE already shows me ...


7

What I don't like about your approach is that the lock objects are never removed from FooServiceCacheLocks, even when the object is removed from the cache. One way to simplify your code would be to combine MemoryCache with Lazy: instances of Lazy are cheap (as long as you don't access its Value) so you can create more of them than needed. With that your ...


7

Let's go over the basics first! The indentation is quite a mess. But I suppose that might be related to the SE formatting. Otherwise, you should make your indentation coherent. Stuff like : public interface IGYDataAccess { void GetUserInfo(Action<GYUser, Exception> act, string id); } shouldn't be in production code, it ...


7

Thanks to an answer to a similar question on StackOverflow (thanks @200_success), I improved the implementation of operator(). Using _memory.count is pretty inefficient compared to _memory.find since the latter will stop once the first corresponding key is found: auto operator()(Args... args) -> Ret { const auto t_args = std::make_tuple(args...); ...


7

Instead of having a cache private static Dictionary<string, ObjectCache> cacheStore; you could have one keyed of the types: private static Dictionary<Type, ObjectCache> cacheStore; This means you don't need to call typeof(T).ToString() everywhere. You duplicate the code of building the key in all 3 methods - it should be extracted into a ...


7

If code creates a string, quickly finds out that an identical string exists somewhere (e.g. in a HashMap), and replaces the reference to the new string with a reference to the cached one, then the garbage collector can immediately discard the new string without having to spend any time copying it out of the "Eden" heap. This is a useful optimization when it ...


7

A Dictionary is the wrong data structure to use for memoization here. An ArrayList A List<int> would be more appropriate. The reason is that the entries are not independent, but rather sequential: it will remember all entries from the 0th to some maximum. There's not much point to hashing when a simple array lookup will do. When a value is not in ...


7

From what I understand, the lru_cache is not the right tool when it comes to caching things in a production web application. Depending on how the web application is deployed, you would usually end up having multiple web server processes serving your application. Which means, that the LRU cache will be created in the memory space of every process. It's like ...


7

Code simplification You don't need to use a custom linked list here. std::list will do fine, too. You can keep the pairs (key, value) in an std::list and store a mapping from keys to iterators in this list in an std::unordered_map. Passing by const reference Passing keys and values by value creates an extra copy, which may be expensive. You can pass ...


7

Namespace Usage I'd put all of this into some namespace, so it won't accidentally collide with other usage of the names it defines. I'd probably also work even a little harder to do something to hide your node and Default templates. For example, node doesn't need to be visible to the outside world, so it would probably be better off defined inside of lru. ...


7

Header guards Your code contains undefined behaviour since the underscores are reserved for the implementation: 17.6.4.3 Reserved names [reserved.names] The C++ standard library reserves the following kinds of names: macros global names names with external linkage If a program declares or defines a name in a context where it is ...


7

There is no reason for node to know the key. The node constructor shall make a completely created node, that is prev and next fields should be initialized as well (to nullptr). Member initializer lists are preferred to constructor bodies. For example, lru<K,V>::lru(std::size_t capacity) : capacity(capacity) , prev(nullptr) ,...


7

Sorry this is a bit of a ramble, but a few things jump out at me: 17?!?! I don't have to tell you about magic numbers! This should probably be configurable... somehow... Why is Locker static? It doesn't make sense to restrict access to one cache because a totally different cache is currently in use. Locking the cache while you create an item isn't ...


Only top voted, non community-wiki answers of a minimum length are eligible