Increase IPP_MAX_VALUES #1284

michaelrsweet · 2005-09-29T13:20:42Z

Version: 1.1.23
CUPS.org User: twaugh.redhat

While analysing why CUPS cannot scale to 10000 queues, I discovered that a large amount of time is spent in ippReadIO(), here:

1052 /*
1053 * Reset pointers in the list...
1054 */
1055
1056 for (ptr = ipp->attrs; ptr && ptr->next != attr; ptr = ptr->next);

By increasng IPP_MAX_VALUES in ipp.h from 10 to 100 I avoided this scalability issue altogether.

michaelrsweet · 2005-09-29T17:40:19Z

CUPS.org User: mike

Increasing IPP_MAX_VALUES only reduces the problem, it doesn't eliminate it, and it will increase the amount of memory that is needed for even small requests.

I'll work up a more efficient implementation that caches the pointers so we don't need to do the list traversal all the time.

michaelrsweet · 2005-09-29T20:05:29Z

CUPS.org User: mike

Fixed in Subversion repository.

I've attached a patch that works with CUPS 1.1.23 which adds a "prev" pointer to the ipp_t structure and changes the allocation increment (IPP_MAX_VALUES) to a power of two. The new code initially allocates 1 value and then resizes in IPP_MAX_VALUES increments.

Let me know if you find a larger value of IPP_MAX_VALUES still works better.

FWIW, make sure you also include the patch in STR #1249 (which fixes reading of collections...)

michaelrsweet · 2005-09-30T09:33:02Z

CUPS.org User: twaugh.redhat

Where can I find the patch? I'd like to try it out, but can't find the link to it.

michaelrsweet · 2005-09-30T12:35:14Z

CUPS.org User: mike

Sorry, here it is...

michaelrsweet · 2005-10-05T10:11:51Z

CUPS.org User: twaugh.redhat

Thanks. Also:

FWIW, make sure you also include the patch in STR #1249
(which fixes reading of collections...)

I couldn't see a patch attached to STR #1249 either.

michaelrsweet · 2005-10-05T12:19:41Z

CUPS.org User: mike

Sorry, the patch is attached now...

michaelrsweet · 2006-01-19T03:27:22Z

"str1284.patch":

Index: ipp.c

--- ipp.c (revision 4723)
+++ ipp.c (working copy)
@@ -785,11 +785,17 @@
return (NULL);

if (ipp->current)

attr = ipp->current->next;
{
ipp->prev = ipp->current;
attr = ipp->current->next;
}
else
attr = ipp->attrs;
{
ipp->prev = NULL;
attr = ipp->attrs;
}
for (; attr != NULL; attr = attr->next)
for (; attr != NULL; ipp->prev = attr, attr = attr->next)
{
DEBUG_printf(("ippFindAttribute: attr = %p, name = '%s'\n", attr,
attr->name));
@@ -808,6 +814,7 @@
}

ipp->current = NULL;

ipp->prev = NULL;

return (NULL);
}
@@ -828,12 +835,14 @@

'ippNew()' - Allocate a new IPP request.
*/

-ipp_t * /* O - New IPP request /
+ipp_t * / O - New IPP request */
ippNew(void)
{

ipp_t temp; / New IPP request */
ipp_t temp; / New IPP request */
DEBUG_puts("ippNew()");

if ((temp = (ipp_t )calloc(1, sizeof(ipp_t))) != NULL)
{
/
@@ -844,7 +853,7 @@
temp->request.any.version[1] = 1;
}

DEBUG_printf(("ippNew(): %p\n", temp));
DEBUG_printf(("ippNew: %p\n", temp));

return (temp);
}
@@ -948,16 +957,19 @@
ipp->request.any.op_status = (buffer[2] << 8) | buffer[3];
ipp->request.any.request_id = (((((buffer[4] << 8) | buffer[5]) << 8) |
buffer[6]) << 8) | buffer[7];
+

     DEBUG_printf(("ippReadIO: version=%d.%d\n", buffer[0], buffer[1]));

DEBUG_printf(("ippReadIO: op_status=%04x\n",

              ipp->request.any.op_status));

DEBUG_printf(("ippReadIO: request_id=%d\n",

              ipp->request.any.request_id));
 }

 ipp->state   = IPP_ATTRIBUTE;

ipp->current = NULL;
ipp->curtag = IPP_TAG_ZERO;

ipp->prev = ipp->last;

   DEBUG_printf(("ippReadIO: version=%d.%d\n", buffer[0], buffer[1]));

DEBUG_printf(("ippReadIO: op_status=%04x\n", ipp->request.any.op_status));

- DEBUG_printf(("ippReadIO: request_id=%d\n", ipp->request.any.request_id));

    /*
     * If blocking is disabled, stop here...
*/

@@ -968,6 +980,9 @@
case IPP_ATTRIBUTE :
while ((*cb)(src, buffer, 1) > 0)
{

DEBUG_printf(("ippReadIO: ipp->current=%p, ipp->prev=%p\n",

              ipp->current, ipp->prev));

/*

Read this attribute...
*/
@@ -992,11 +1007,14 @@
*/
```
 if (ipp->curtag == tag)
```

```
    ippAddSeparator(ipp);
```
```
    ipp->prev = ippAddSeparator(ipp);
```
```
       else if (ipp->current)
```

    ipp->prev = ipp->current;

ipp->curtag  = tag;
ipp->current = NULL;

  DEBUG_printf(("ippReadIO: group tag = %x\n", tag));

  DEBUG_printf(("ippReadIO: group tag = %x, ipp->prev=%p\n", tag,

```
                ipp->prev));
continue;
```
}

@@ -1022,7 +1040,8 @@

       DEBUG_printf(("ippReadIO: name length = %d\n", n));

     if (n == 0 && tag != IPP_TAG_MEMBERNAME)

     if (n == 0 && tag != IPP_TAG_MEMBERNAME &&

```
    tag != IPP_TAG_END_COLLECTION)
```
{
/*
* More values for current attribute...
@@ -1059,27 +1078,29 @@
(tag < IPP_TAG_TEXTLANG || tag > IPP_TAG_MIMETYPE))
return (IPP_ERROR);
}
```
  else if (attr->value_tag != tag &&
```
```
       tag != IPP_TAG_END_COLLECTION)
```

  else if (attr->value_tag != tag)
  return (IPP_ERROR);

    /*
* Finally, reallocate the attribute array as needed...
*/

  if ((attr->num_values % IPP_MAX_VALUES) == 0 &&

```
      attr->num_values > 0)
```
```
  if (attr->num_values == 1 ||
```
```
      (attr->num_values > 0 &&
```

       (attr->num_values & (IPP_MAX_VALUES - 1)) == 0))
{

    ipp_attribute_t   _temp,  /_ Pointer to new buffer */

          _ptr;   /_ Pointer in attribute list */

    ipp_attribute_t   *temp;  /* Pointer to new buffer */

         DEBUG_printf(("ippReadIO: reallocating for up to %d values...\n",

                  attr->num_values + IPP_MAX_VALUES));

```
    /*
\* Reallocate memory...
*/
```

       if ((temp = realloc(attr, sizeof(ipp_attribute_t) +

                              (attr->num_values + IPP_MAX_VALUES) *

                              (attr->num_values + IPP_MAX_VALUES - 1) *
            sizeof(ipp_value_t))) == NULL)
    return (IPP_ERROR);

@@ -1089,10 +1110,8 @@
* Reset pointers in the list...
*/

- for (ptr = ipp->attrs; ptr && ptr->next != attr; ptr = ptr->next);

```
      if (ptr)
```
```
        ptr->next = temp;
```
```
      if (ipp->prev)
```

        ipp->prev->next = temp;
else
      ipp->attrs = temp;

@@ -1112,13 +1131,19 @@
return (IPP_ERROR);
}

  attr = ipp->current = _ipp_add_attr(ipp, IPP_MAX_VALUES);

```
       if (ipp->current)
```
```
    ipp->prev = ipp->current;
```

  attr = ipp->current = _ipp_add_attr(ipp, 1);

  DEBUG_printf(("ippReadIO: membername, ipp->current=%p, ipp->prev=%p\n",

```
                ipp->current, ipp->prev));
```
- attr->group_tag = ipp->curtag;
  attr->value_tag = IPP_TAG_ZERO;
  attr->num_values = 0;
  }
```
else
```
```
else if (tag != IPP_TAG_END_COLLECTION)
```
{
/*
* New attribute; read the name and add it...
@@ -1131,17 +1156,23 @@
}
```
buffer[n] = '\0';
```

  DEBUG_printf(("ippReadIO: name = \'%s\'\n", buffer));

  attr = ipp->current = _ipp_add_attr(ipp, IPP_MAX_VALUES);

```
       if (ipp->current)
```
```
    ipp->prev = ipp->current;
```

  attr = ipp->current = _ipp_add_attr(ipp, 1);

  DEBUG_printf(("ippReadIO: name=\'%s\', ipp->current=%p, ipp->prev=%p\n",

```
                buffer, ipp->current, ipp->prev));
```
- attr->group_tag = ipp->curtag;
  attr->value_tag = tag;
  attr->name = strdup((char *)buffer);
  attr->num_values = 0;
  }

     value = attr->values + attr->num_values;

     if (tag != IPP_TAG_END_COLLECTION)

```
       value = attr->values + attr->num_values;
```
if ((*cb)(src, buffer, 2) < 2)
{
@@ -2252,16 +2283,16 @@

attr->num_values = num_values;
if (attr == NULL)
return (NULL);
if (attr != NULL)
{
if (ipp->last == NULL)
```
 ipp->attrs = attr;
```
else
```
 ipp->last->next = attr;
```
if (ipp->last == NULL)
ipp->attrs = attr;
else
ipp->last->next = attr;
ipp->last = attr;
}

- ipp->last = attr;

DEBUG_printf(("_ipp_add_attr(): %p\n", attr));

return (attr);
@@ -2432,11 +2463,7 @@
for (i = 0, value = attr->values;
i < attr->num_values;
i ++, value ++)

```
{
```
-/* bytes += 5;/ / Overhead of begCollection /
bytes += ipp_length(attr->values[i].collection, 1);
-/ bytes += 5;/ / Overhead of endCollection */
```
}
```
break;

default :

Index: ipp.h

--- ipp.h (revision 4723)
+++ ipp.h (working copy)
@@ -65,7 +65,7 @@
*/

define IPP_MAX_NAME 256

-# define IPP_MAX_VALUES 10 /* Now just an allocation increment /
+# define IPP_MAX_VALUES 8 / Power-of-2 allocation increment */

/*
@@ -400,6 +400,9 @@
last, / Last attribute in list /
*current; / Current attribute (for read/write) /
ipp_tag_t curtag; / Current attribute group tag /
+
+/*** New in CUPS 1.2 ****/
ipp_attribute_t prev; / Previous attribute (for read) */
};

michaelrsweet closed this as completed Jan 19, 2006

michaelrsweet added the priority-low label Mar 17, 2016

michaelrsweet added this to the Stable milestone Mar 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase IPP_MAX_VALUES #1284

Increase IPP_MAX_VALUES #1284

michaelrsweet commented Sep 29, 2005

michaelrsweet commented Sep 29, 2005

michaelrsweet commented Sep 29, 2005

michaelrsweet commented Sep 30, 2005

michaelrsweet commented Sep 30, 2005

michaelrsweet commented Oct 5, 2005

michaelrsweet commented Oct 5, 2005

michaelrsweet commented Jan 19, 2006

Index: ipp.h

define IPP_MAX_NAME 256

Increase IPP_MAX_VALUES #1284

Increase IPP_MAX_VALUES #1284

Comments

michaelrsweet commented Sep 29, 2005

michaelrsweet commented Sep 29, 2005

michaelrsweet commented Sep 29, 2005

michaelrsweet commented Sep 30, 2005

michaelrsweet commented Sep 30, 2005

michaelrsweet commented Oct 5, 2005

michaelrsweet commented Oct 5, 2005

michaelrsweet commented Jan 19, 2006

Index: ipp.c

- DEBUG_printf(("ippReadIO: request_id=%d\n", ipp->request.any.request_id));

- for (ptr = ipp->attrs; ptr && ptr->next != attr; ptr = ptr->next);

- ipp->last = attr;

Index: ipp.h

define IPP_MAX_NAME 256