[Webkit-unassigned] [Bug 196094] [GTK][WPE] Implement a basic DNS cache
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 22 03:23:42 PDT 2019
https://bugs.webkit.org/show_bug.cgi?id=196094
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |cgarcia at igalia.com
Attachment #365571|review? |review-
Flags| |
--- 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_RESOLVER,
> + "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
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190322/d462e941/attachment-0001.html>
More information about the webkit-unassigned
mailing list