[webkit-reviews] review denied: [Bug 196094] [GTK][WPE] Implement a basic DNS cache : [Attachment 365571] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Patrick Griffis
<pgriffis at igalia.com>'s request for review:
Bug 196094: [GTK][WPE] Implement a basic DNS cache
https://bugs.webkit.org/show_bug.cgi?id=196094

Attachment 365571: Patch

https://bugs.webkit.org/attachment.cgi?id=365571&action=review




--- Comment #6 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 365571
  --> https://bugs.webkit.org/attachment.cgi?id=365571
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=365571&action=review

Great! This patch is perfect for libsoup, but not for WebKit. We should follow
the WebKit coding style, use smart pointers and our internal data structures
from WTF whenever possible. I'll add more specific comments in the patch.

> Source/WebKit/NetworkProcess/soup/NetworkProcessSoup.cpp:115
> +    webkit_soup_cached_resolver_ensure_default();

We know this is called only once so we could simply:

GRefPtr<GResolver> resolver =
webkitCachedResolverNew(adoptGRef(g_resolver_get_default()));
g_resolver_set_default(resolver.get());

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:27
> +#include "WebKitSoupCachedResolver.h"

This is not soup specific art all, so I would avoid the Soup in the name, I
would just call this WebKitCachedResolver.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:45
> + */

This is private object in WebKit, so we don't need documentation. If you want
to keep a comment explaining what this object is for, remove the gtk-doc tags
and use C++ style comment.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:48
> +/* This version introduces querying names for ipv4/ipv6
> + * separately which we cache separately */

Use C++ style comment. It could be one line and finish with period.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:55
> +struct _WebKitSoupCachedResolver {

It's fine to keep the instance and class structs private here but we also need
to use a private instance to be able to use smart pointers and C++ objects.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:56
> +    GResolver parent_instance;

parentInstance

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:57
> +    GResolver* wrapped_resolver;

GRefPtr<GResolver> wrappedResolver;

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:58
> +    GHashTable* dns_caches[N_CACHES];

Use HashMap from WTF instead of GHashTable. It could be

HashMap<CString, CachedResponse>

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:59
> +    GMutex dns_cache_lock;

Use WTF Lock

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:63
> +    GResolverClass parent_class;

parentClass

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:66
> +G_DEFINE_TYPE(WebKitSoupCachedResolver, webkit_soup_cached_resolver,
G_TYPE_RESOLVER)

Use WebKitDefineType

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:68
> +/* These match Firefox */

C++ style comments

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:70
> +#define DNS_CACHE_EXPIRE_SECONDS 60
> +#define DNS_CACHE_MAX_SIZE 400

static const Seconds dnsCachedExpireSeconds = 60_s;
static const unsigned dnsCacheMaxSize = 400;

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:88
> +/**
> + * webkit_soup_cached_resolver_new:
> + * @wrapped_resolver: Underlying #GResolver to be cached
> + * 
> + * Note that @wrapped_resolver must not be a #WebKitSoupCachedResolver.
> + *
> + * You should generally use webkit_soup_cached_resolver_ensure_default()
> + * rather than this API directly.
> + *
> + * Returns:(transfer full): A new #WebKitSoupCachedResolver
> + */

Same comment here about the api docs.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:90
> +static WebKitSoupCachedResolver* 
> +webkit_soup_cached_resolver_new(GResolver* wrapped_resolver)

This should be one line. I would make this public instead of
webkit_soup_cached_resolver_ensure_default().

GResolver* webkitCachedResolverNew(GRefPtr<GResolver>&& wrappedResolver)

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:92
> +    g_return_val_if_fail(wrapped_resolver, nullptr);

G_IS_RESOLVER(wrapperResolver.get(), nullptr);

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:93
> +    g_return_val_if_fail(!WEBKIT_IS_SOUP_CACHED_RESOLVER(wrapped_resolver),
nullptr);

We wouldn't need this check, replace it with ASSERT if you really want to check
this.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:97
> +    return
static_cast<WebKitSoupCachedResolver*>(g_object_new(WEBKIT_TYPE_SOUP_CACHED_RES
OLVER,
> +	   "wrapped-resolver", wrapped_resolver,
> +	   nullptr));

Since this is private object and won't be used by any binding, we can get rid
of the construct property and assign the wrapped object here after g_object_new
call.

auto* resolver =
static_cast<WebKitCachedResolver*>(g_object_new(WEBKIT_TYPE_CACHED_RESOLVER,
nullptr));
resolver->priv->wrappedResolver = WTFMove(wrappedResolver);

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:123
> +typedef struct {
> +    GList* addresses; /* owned */
> +    gint64 expiration;
> +} CachedResponse;

struct CachedResponse {
    Vector<GRefPtr<GInetAddress>> addresses;
    WallTime expiration;
};

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:130
> +static void
> +cached_response_free(CachedResponse* cache)
> +{
> +    g_resolver_free_addresses(cache->addresses);
> +    g_free(cache);
> +}

Then you don't need this.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:170
> +static gpointer
> +copy_object(gconstpointer obj, gpointer user_data)
> +{
> +    return g_object_ref(G_OBJECT(obj));
> +}
> +
> +static GList* 
> +copy_addresses(GList* addresses)
> +{
> +    return g_list_copy_deep(addresses, copy_object, nullptr);
> +}

You shouldn't need this either.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:177
> +    gint64 now = g_get_monotonic_time();

Seconds now = WallTime::now();

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:184
> +	   if (cached->expiration <= now || size > DNS_CACHE_MAX_SIZE)

How can be the current cache bigger than the maximum size? Shouldn't we check
the expiration time to decide what response to remove when the cache is full?

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:208
> +    /* Cleanup while we are at it. */
> +    cleanup_dns_cache(self, cache);

I think we could split the case of cache being full from the case of expired
responses. Should we use a timer to remove expired resources instead of doing
it only when adding new entries?

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:234
> +    if (cached && cached->expiration > now)
> +	   addresses = copy_addresses(cached->addresses);

Shouldn't we remove the entry from the cache if it has expired?

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:240
> +

I think we can add a private C++ class here to handle the cache and make the
GObject use it.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:257
> +typedef struct {
> +    char* hostname;
> +    GResolverNameLookupFlags flags;
> +} LookupData;

struct LookupAsyncData {
    GUniquePtr<char> hostname;
    GResolverNameLookupFlags flags;
};
WEBKIT_DEFINE_ASYNC_DATA_STRUCT(LookupAsyncData);

That will define createLookupAsyncData() and destroyLookupAsyncData() function
using fast malloc and allowing to use smart pointers.

> Source/WebKit/Shared/glib/WebKitSoupCachedResolver.cpp:325
> +    GTask* task = g_task_new(self, cancellable, callback, user_data);

GRefPtr<GTask> task = adoptGRef()

> Source/WebKit/SourcesGTK.txt:92
> +Shared/glib/WebKitSoupCachedResolver.cpp

This doesn't belong to Shared. I Shared we have things shared by multiple
processes, but the dns cache is only used in the network process, so this
should be in NetworkProcess/glib or NetworkProcess/soup