From: "Bill Stoddard" <[EMAIL PROTECTED]>
Sent: Tuesday, August 14, 2001 4:26 PM
> > This is getting obscene. We've lost all value of tables, since they can't be
>memcpy'ed,
> > they have to be reconstructed. This will be much slower than the original array
>code.
The option, Bill, to eliminate the hash entirely, is still in the realm of
consideration.
But that patch can't undo Roy's and my bug fixes (that weren't related to the hash.)
> > I'll demonstrate a solution, using pool scopes, that adds safety and speed. Give
>me a day.
>
> Hit send before I was finished. If pool scopes significantly increases the
>complexity of
> this code, I will likely veto it. So you might want to share what you are planning
>on
> doing before you invest much time in it. I learned a lesson working on this code...
>the
> dir_create() and dir_merge() code needs to be as straight forward and easy to
>understand
> as possible to keep bugs out. Gaining 5% performance at the expense of a 100%
>increase in
> code complexity is not an acceptable trade off. The copy_mappings, et. al. was more
>like a
> 1000% increase in complexity.
OK. More complex, and not 'tweaked' for legibility. Rather than make you all wait,
I'm posting as-is, and won't commit yet, since we have some contention about the
'right'
solution.
The list of 'optimizable cases' shrinks in this patch. ->copy/copy_mappings is gone.
We only care that the cfg->pool and entry->pool are in-scope. If they aren't, we copy.
We've got to copy when we merge or remove. OH ... this patch defers the forced
'hash_make' until we actually add an entry. You may want to ignore that change (set
new->extension_mappings = apr_hash_make(p); in line 152) to continue to reproduce the
segfault reliably.
remove-items is kluged to eliminate a ton of lines (three reps). Less prone to error,
I hope. The big headache is the 'member' index into the exinfo structure. That
'offset'
code gets ugly, perhaps someone has a better/cleaner solution (using a macro or some
such.)
Anyways, I've wrapped my brain around this another three hours. Try this patch if you
can
reproduce the segfault (I can't, but couldn't before) and report back.
Bill
Index: modules/http/mod_mime.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/mod_mime.c,v
retrieving revision 1.50
diff -u -r1.50 mod_mime.c
--- modules/http/mod_mime.c 2001/08/14 02:35:55 1.50
+++ modules/http/mod_mime.c 2001/08/15 02:36:26
@@ -105,13 +105,13 @@
char *language_type; /* Added with AddLanguage... */
char *handler; /* Added with AddHandler... */
char *charset_type; /* Added with AddCharset... */
- int copy; /* To avoid dupping in the merge */
+ apr_pool_t *pool; /* To dup appropriately in the merge */
} extension_info;
typedef struct {
apr_hash_t *extension_mappings; /* Map from extension name to
* extension_info structure */
- int copy_mappings; /* To avoid dupping in the merge */
+ apr_pool_t *pool; /* To dup appropriately in the merge */
apr_array_header_t *handlers_remove; /* List of handlers to remove */
apr_array_header_t *types_remove; /* List of MIME types to remove */
@@ -149,8 +149,8 @@
mime_dir_config *new =
(mime_dir_config *) apr_palloc(p, sizeof(mime_dir_config));
- new->extension_mappings = apr_hash_make(p);
- new->copy_mappings = 0;
+ new->pool = p;
+ new->extension_mappings = NULL;
new->handlers_remove = NULL;
new->types_remove = NULL;
@@ -179,13 +179,20 @@
base_info = (extension_info*)apr_hash_get(base, key, klen);
- if (base_info) {
- if (!base_info->copy) {
+ /* If we don't find the entry, simply add it (as is).
+ * If we find the entry, and this exinfo wasn't allocated from the
+ * current pool, then we must copy before can merge the entry.
+ */
+ if (!base_info) {
+ apr_hash_set(base, key, klen, overlay_info);
+ }
+ else {
+ if (base_info->pool != p) {
extension_info *copyinfo = base_info;
base_info = (extension_info*)apr_palloc(p, sizeof(*base_info));
apr_hash_set(base, key, klen, base_info);
memcpy(base_info, copyinfo, sizeof(*base_info));
- base_info->copy = 1;
+ base_info->pool = p;
}
if (overlay_info->forced_type) {
@@ -204,8 +211,34 @@
base_info->charset_type = overlay_info->charset_type;
}
}
- else {
- apr_hash_set(base, key, klen, overlay_info);
+ }
+}
+
+
+static void remove_items(apr_pool_t *p, apr_array_header_t *remove,
+ apr_hash_t *mappings, apr_size_t member)
+{
+ attrib_info *suffix = (attrib_info *) remove->elts;
+ int i;
+ for (i = 0; i < remove->nelts; i++) {
+ extension_info *exinfo =
+ (extension_info*)apr_hash_get(mappings,
+ suffix[i].name,
+ APR_HASH_KEY_STRING);
+ /* If we find the entry, the given member isn't NULL,
+ * and this exinfo wasn't allocated from the current pool,
+ * then we must copy before we remove the given member.
+ */
+ if (exinfo && *((void **)((char*)&exinfo + member))) {
+ if (exinfo->pool != p) {
+ extension_info *copyinfo = exinfo;
+ exinfo = (extension_info*)apr_palloc(p, sizeof(*exinfo));
+ apr_hash_set(mappings, suffix[i].name,
+ APR_HASH_KEY_STRING, exinfo);
+ memcpy(exinfo, copyinfo, sizeof(*exinfo));
+ exinfo->pool = p;
+ }
+ *(void **)((char*)&exinfo + member) = NULL;
}
}
}
@@ -215,106 +248,52 @@
mime_dir_config *base = (mime_dir_config *) basev;
mime_dir_config *add = (mime_dir_config *) addv;
mime_dir_config *new = apr_palloc(p, sizeof(mime_dir_config));
-
- int i;
- attrib_info *suffix;
+ extension_info dummyex;
+ new->pool = p;
- if (base->extension_mappings && add->extension_mappings) {
- if (base->copy_mappings)
- new->extension_mappings = base->extension_mappings;
- else {
- new->extension_mappings = apr_hash_make(p);
+ /* Given no mappings, we set up no mappings table.
+ * If we are using the same pool as the base cfg, _or_
+ * we neither have anything to add or remove, we
+ * then we are safe retaining the existing hash.
+ * Otherwise we need a private copy of the hash table.
+ */
+ if (!base->extension_mappings && !add->extension_mappings)
+ new->extension_mappings = NULL;
+ else if (base->extension_mappings
+ && ((base->pool == p) || !(add->extension_mappings
+ || add->handlers_remove
+ || add->types_remove
+ || add->encodings_remove)))
+ new->extension_mappings = base->extension_mappings;
+ else {
+ new->extension_mappings = apr_hash_make(p);
+ if (base->extension_mappings)
overlay_extension_mappings(p, base->extension_mappings,
new->extension_mappings);
- }
+ }
+
+ /* Now we can add and remove hash table entries. These functions
+ * are given a 'safe' hash they can modify, but they have to be
+ * careful to dup the actual hash entry if it isn't allocated from
+ * the given pool!
+ */
+ if (add->extension_mappings)
overlay_extension_mappings(p, add->extension_mappings,
new->extension_mappings);
- new->copy_mappings = 1;
- }
- else {
- if (base->extension_mappings == NULL) {
- new->extension_mappings = add->extension_mappings;
- new->copy_mappings = add->copy_mappings;
- }
- else if (add->extension_mappings == NULL) {
- new->extension_mappings = base->extension_mappings;
- new->copy_mappings = base->copy_mappings;
- }
- if (!new->copy_mappings && (add->handlers_remove
- || add->types_remove
- || add->encodings_remove)) {
- apr_hash_t *copyhash = new->extension_mappings;
- new->extension_mappings = apr_hash_make(p);
- /* ### as slow as can be... just use an apr_hash_dup! */
- overlay_extension_mappings(p, copyhash, new->extension_mappings);
- new->copy_mappings = 1;
- }
- }
- if (add->handlers_remove) {
- suffix = (attrib_info *) add->handlers_remove->elts;
- for (i = 0; i < add->handlers_remove->nelts; i++) {
- extension_info *exinfo =
- (extension_info*)apr_hash_get(new->extension_mappings,
- suffix[i].name,
- APR_HASH_KEY_STRING);
- if (exinfo) {
- if (!exinfo->copy) {
- extension_info *copyinfo = exinfo;
- exinfo = (extension_info*)apr_palloc(p, sizeof(*exinfo));
- apr_hash_set(new->extension_mappings, suffix[i].name,
- APR_HASH_KEY_STRING, exinfo);
- memcpy(exinfo, copyinfo, sizeof(*exinfo));
- exinfo->copy = 1;
- }
- exinfo->handler = NULL;
- }
- }
- }
+ if (add->handlers_remove)
+ remove_items(p, add->handlers_remove, new->extension_mappings,
+ (char*)&dummyex.handler - (char*)&dummyex);
new->handlers_remove = NULL;
- if (add->types_remove) {
- suffix = (attrib_info *) add->types_remove->elts;
- for (i = 0; i < add->types_remove->nelts; i++) {
- extension_info *exinfo =
- (extension_info*)apr_hash_get(new->extension_mappings,
- suffix[i].name,
- APR_HASH_KEY_STRING);
- if (exinfo) {
- if (!exinfo->copy) {
- extension_info *copyinfo = exinfo;
- exinfo = (extension_info*)apr_palloc(p, sizeof(*exinfo));
- apr_hash_set(new->extension_mappings, suffix[i].name,
- APR_HASH_KEY_STRING, exinfo);
- memcpy(exinfo, copyinfo, sizeof(*exinfo));
- exinfo->copy = 1;
- }
- exinfo->forced_type = NULL;
- }
- }
- }
+ if (add->types_remove)
+ remove_items(p, add->types_remove, new->extension_mappings,
+ (char*)&dummyex.forced_type - (char*)&dummyex);
new->types_remove = NULL;
- if (add->encodings_remove) {
- suffix = (attrib_info *) add->encodings_remove->elts;
- for (i = 0; i < add->encodings_remove->nelts; i++) {
- extension_info *exinfo =
- (extension_info*)apr_hash_get(new->extension_mappings,
- suffix[i].name,
- APR_HASH_KEY_STRING);
- if (exinfo) {
- if (!exinfo->copy) {
- extension_info *copyinfo = exinfo;
- exinfo = (extension_info*)apr_palloc(p, sizeof(*exinfo));
- apr_hash_set(new->extension_mappings, suffix[i].name,
- APR_HASH_KEY_STRING, exinfo);
- memcpy(exinfo, copyinfo, sizeof(*exinfo));
- exinfo->copy = 1;
- }
- exinfo->encoding_type = NULL;
- }
- }
- }
+ if (add->encodings_remove)
+ remove_items(p, add->encodings_remove, new->extension_mappings,
+ (char*)&dummyex.encoding_type - (char*)&dummyex);
new->encodings_remove = NULL;
new->type = add->type ? add->type : base->type;
@@ -337,14 +316,16 @@
#endif
if (*key == '.')
++key;
+ if (!m->extension_mappings)
+ m->extension_mappings = apr_hash_make(cmd->pool);
exinfo = (extension_info*)apr_hash_get(m->extension_mappings, key,
APR_HASH_KEY_STRING);
if (!exinfo) {
- /* pcalloc sets ->copy to false */
exinfo = apr_pcalloc(cmd->pool, sizeof(extension_info));
key = apr_pstrdup(cmd->pool, key);
apr_hash_set(m->extension_mappings, key,
APR_HASH_KEY_STRING, exinfo);
+ exinfo->pool = cmd->pool;
}
return exinfo;
}
