From: "Greg Ames" <[EMAIL PROTECTED]>
Sent: Wednesday, August 15, 2001 4:21 PM
> [EMAIL PROTECTED] wrote:
> >
> > wrowe 01/08/14 20:09:33
> >
> > Modified: modules/http mod_mime.c
> > Log:
> > Solve the segfault until the right patch is unearthed.
> > [Greg Ames]
[the right patch is attached]
> slower doing directory merges during config file parsing perhaps (who
> cares?), but faster while serving most requests due Brian's hash stuff.
I care, since some mechansims (think subrequests) -may- generate hundreds of
merges. The nature of rebuilding hashes also has exponential side effects.
That wasn't the only remaining segfault. Mucking with a specific exinfo entry
within a subreq config would STILL cause segfaults in the request or a later subreq.
(Think in terms of includes, adding an incremental .shtml on a first include'ed file,
so when the second include'ed file is loaded the segfault is triggered.)
It has to be done right, or return to tables :(
Would you please apply the attached patch on Daladeus and let me know if the
segfaults are gone, while I work up documentation of the rules of when/how one
can optimize their directory-merge? Without an agreed framework, this will never
happen.
> The config can be dynamically updated during request/subrequest
> processing due to .htaccess files or <File *.asc> containers, maybe a
> few other cases. You want to avoid .htaccess anyway for fastest
> performance (see http://www.apache.org/misc/perf-tuning.html if you're
> not convinced). Plus, it seems like whenever the config is being
> updated dynamically, there won't be a whole lot of cases where we don't
> need a new copy of the hash table.
Huh? I'll post a seperate document describing the gains.
> Brian's suggestion to cache the results of directory merges sounds like
> a more general solution to the problem, assuming there really is a
> problem.
Yes, yes, yes :) But that doesn't prevent us from doing the 'right thing' on both
counts.
Let's say we set up a max of 100 most-frequent directories for caching. On a huge
webserver,
that leaves many directory walks incomplete (or never started.)
> Have directory merges actually shown up on the radar screen
> while doing profiling?
Of course, that was why Brian offered up the original (unfortunately broken) patch.
Optimization is confusing, that's why I'm going to write a short, clear description
of the do's and dont's so we and module authors start from the same assumptions.
> p.s. if somebody wants to make autoindexes run a lot faster (not sure I
> care), why don't we cache which icon to present for a given file? I
> mention autoindexes because that's what triggered this bug.
Because it's already there, and if this patch cleans up the behavioral problems, it
should
be extremely fast. Icons are allowed to vary by <Directory>, <File>, and <Location>
contexts.
> On daedalus, that ends up driving mod_mime_magic for many files every
> single time a browser accesses a distribution directory. This generates
> tons of overhead, including gunzip'ing at least part of every .gz file
> in the directory. I suppose we should try to shake out bugs in m_m_m,
> but we can do that without constantly driving it to choose the autoindex
> icons.
Caching won't fix that. We should look at which requests fall back on mod_mime_magic,
it
really shouldn't be hit so often :( I'll work up some info-level logging patch for
m_m_m since
it's such a performance killer. Autoindex works up a subrequest for many reasons,
even checking
that the file can be served. If you kill the subrequests from autoindex, we will
loose the
appearance of security, even if the actual files can't be comprimized (easily.)
Bill
Index: modules/http/mod_mime.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/mod_mime.c,v
retrieving revision 1.51
diff -u -r1.51 mod_mime.c
--- modules/http/mod_mime.c 2001/08/15 03:09:32 1.51
+++ modules/http/mod_mime.c 2001/08/15 22:28:08
@@ -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));
+ extension_info dummyex;
+ new->pool = p;
- int i;
- attrib_info *suffix;
-
- if (base->extension_mappings && add->extension_mappings) {
- /* XXX: Greg Ames' fast hack to always copy the hash.
- * It's incomplete, and slower than tables, but at least
- * the server (mostly) runs once again.
- */
- 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,19 @@
#endif
if (*key == '.')
++key;
- exinfo = (extension_info*)apr_hash_get(m->extension_mappings, key,
- APR_HASH_KEY_STRING);
+ if (!m->extension_mappings) {
+ m->extension_mappings = apr_hash_make(cmd->pool);
+ exinfo = NULL;
+ }
+ else
+ 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;
}
