Re: [2/2]dwrite: basic implementation for FontCollectionLoader registration

[Nikolay Sivov]

> +    IDWriteFontCollectionLoader **CollectionLoaders;
> +    LONG CollectionLoaderCount;

I'd really like a better naming for those, something like 'loaders' and 
'loaders_count'.
> +    if (i == This->CollectionLoaderCount)
> +    {
> +        This->CollectionLoaderCount++;
> +        if (This->CollectionLoaders)
> +            This->CollectionLoaders = heap_realloc(This->CollectionLoaders, This->CollectionLoaderCount * sizeof(IDWriteFontCollectionLoader*));
> +        else
> +            This->CollectionLoaders = heap_alloc(This->CollectionLoaderCount * sizeof(IDWriteFontCollectionLoader*));
> +        if (!This->CollectionLoaders)
> +            return E_OUTOFMEMORY;
> +    }
> +    IDWriteFontCollectionLoader_AddRef(loader);
> +    This->CollectionLoaders[i] = loader;

This should grow more efficiently in my opinion. Also it probably makes 
sense to have small number of slots allocated on initial factory 
creation or even use a list (you don't need index access). Another 
question is do we want to register a default local loader on factory 
creation or not.

[2/2]dwrite: basic implementation for FontCollectionLoader registration

[Aric Stewart]

On 8/1/14, 1:48 PM, Nikolay Sivov wrote:
> > +    IDWriteFontCollectionLoader **CollectionLoaders;
> > +    LONG CollectionLoaderCount;
>
> I'd really like a better naming for those, something like 'loaders' and 'loaders_count'.

Easy to do.
> > +    if (i == This->CollectionLoaderCount)
> > +    {
> > +        This->CollectionLoaderCount++;
> > +        if (This->CollectionLoaders)
> > +            This->CollectionLoaders = heap_realloc(This->CollectionLoaders, This->CollectionLoaderCount * sizeof(IDWriteFontCollectionLoader*));
> > +        else
> > +            This->CollectionLoaders = heap_alloc(This->CollectionLoaderCount * sizeof(IDWriteFontCollectionLoader*));
> > +        if (!This->CollectionLoaders)
> > +            return E_OUTOFMEMORY;
> > +    }
> > +    IDWriteFontCollectionLoader_AddRef(loader);
> > +    This->CollectionLoaders[i] = loader;
>
> This should grow more efficiently in my opinion. Also it probably makes sense to have small number of slots allocated on initial factory creation or even use a list (you don't need index access). Another question is do we want to register a default local loader on factory creation or not.

Do we have any evidence that an application will realistically use more than 1 or 2 of these.  I guess we will have to check with what Word 2013 does because it is likely that would be the worse case. I am guessing that 90% of the applications using direct write do not use this at all.  So preallocating feels like it would mostly be a waste of memory.

-aric

[2/2]dwrite: basic implementation for FontCollectionLoader registration

[Nikolay Sivov]

On 08/04/2014 05:30 PM, Aric Stewart wrote:
> On 8/1/14, 1:48 PM, Nikolay Sivov wrote:
> > > +    IDWriteFontCollectionLoader **CollectionLoaders;
> > > +    LONG CollectionLoaderCount;
> >
> > I'd really like a better naming for those, something like 'loaders' and 'loaders_count'.
>
> Easy to do.
> > > +    if (i == This->CollectionLoaderCount)
> > > +    {
> > > +        This->CollectionLoaderCount++;
> > > +        if (This->CollectionLoaders)
> > > +            This->CollectionLoaders = heap_realloc(This->CollectionLoaders, This->CollectionLoaderCount * sizeof(IDWriteFontCollectionLoader*));
> > > +        else
> > > +            This->CollectionLoaders = heap_alloc(This->CollectionLoaderCount * sizeof(IDWriteFontCollectionLoader*));
> > > +        if (!This->CollectionLoaders)
> > > +            return E_OUTOFMEMORY;
> > > +    }
> > > +    IDWriteFontCollectionLoader_AddRef(loader);
> > > +    This->CollectionLoaders[i] = loader;
> >
> > This should grow more efficiently in my opinion. Also it probably makes sense to have small number of slots allocated on initial factory creation or even use a list (you don't need index access). Another question is do we want to register a default local loader on factory creation or not.
>
> Do we have any evidence that an application will realistically use more than 1 or 2 of these.  I guess we will have to check with what Word 2013 does because it is likely that would be the worse case. I am guessing that 90% of the applications using direct write do not use this at all.  So preallocating feels like it would mostly be a waste of memory.

No, I don't know if anything is using that. If it turns out that in most 
cases it doesn't exceed certain size we could make it an initial 
allocation size. My concern was about local loader, if it's about to be 
stored in same array then it makes sense to always allocate a bit of slots.
> -aric

[2/2]dwrite: basic implementation for FontCollectionLoader registration

[Aric Stewart]

On 8/4/14, 8:37 AM, Nikolay Sivov wrote:
> On 08/04/2014 05:30 PM, Aric Stewart wrote:
> > On 8/1/14, 1:48 PM, Nikolay Sivov wrote:
> > > > +    IDWriteFontCollectionLoader **CollectionLoaders;
> > > > +    LONG CollectionLoaderCount;
> > >
> > > I'd really like a better naming for those, something like 'loaders' and 'loaders_count'.
> >
> > Easy to do.
> > > > +    if (i == This->CollectionLoaderCount)
> > > > +    {
> > > > +        This->CollectionLoaderCount++;
> > > > +        if (This->CollectionLoaders)
> > > > +            This->CollectionLoaders = heap_realloc(This->CollectionLoaders, This->CollectionLoaderCount * sizeof(IDWriteFontCollectionLoader*));
> > > > +        else
> > > > +            This->CollectionLoaders = heap_alloc(This->CollectionLoaderCount * sizeof(IDWriteFontCollectionLoader*));
> > > > +        if (!This->CollectionLoaders)
> > > > +            return E_OUTOFMEMORY;
> > > > +    }
> > > > +    IDWriteFontCollectionLoader_AddRef(loader);
> > > > +    This->CollectionLoaders[i] = loader;
> > >
> > > This should grow more efficiently in my opinion. Also it probably makes sense to have small number of slots allocated on initial factory creation or even use a list (you don't need index access). Another question is do we want to register a default local loader on factory creation or not.
> >
> > Do we have any evidence that an application will realistically use more than 1 or 2 of these.  I guess we will have to check with what Word 2013 does because it is likely that would be the worse case. I am guessing that 90% of the applications using direct write do not use this at all.  So preallocating feels like it would mostly be a waste of memory.
>
> No, I don't know if anything is using that. If it turns out that in most cases it doesn't exceed certain size we could make it an initial allocation size. My concern was about local loader, if it's about to be stored in same array then it makes sense to always allocate a bit of slots.

My thought is that our IDWriteLocalFontFileLoader implementation would not be in this array. It is accessed through a completely different method in the factory (IDWriteFactory::GetSystemFontCollection) so it seemed to make sense to have it be separate.

-aric