Skip to content

Move impure-related string allocation to helper methods #8609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dyemanov merged 4 commits into from
Jun 27, 2025
Merged

Move impure-related string allocation to helper methods #8609

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f459bb
Move impure-related string allocation to helper methods
Jun 18, 2025
4f62040
Use better name for impure string make method
Jun 19, 2025
5b9af0e
Check dsc_length via type_lengths array
Jun 19, 2025
ccc9f51
Use different names for new impure helper methods
Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/common/dsc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,11 @@ typedef struct dsc
return dsc_dtype == dtype_unknown;
}

bool hasDynamicLength() const
{
return isText(); // TODO: add isJson();
}

Copy link
Member

@dyemanov dyemanov Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not look proper to me, as CHAR is not dynamic-length generally (except inside the impure). IMHO, it should be up to the caller (impure class) to check what string types are stored as variable-length.

Copy link
Contributor Author

@Noremos Noremos Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By this method I mean that dsc_length of content is not fixed. Now I remembered that there is an array type_lengths that checks exactly this, so I replace this method with the array

SSHORT getBlobSubType() const
{
if (isBlob())
Expand Down
68 changes: 3 additions & 65 deletions src/dsql/ExprNodes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3752,25 +3752,8 @@ dsc* CastNode::perform(thread_db* tdbb, impure_value* impure, dsc* value,
impure->vlu_desc.dsc_length = length;
}

length = impure->vlu_desc.dsc_length;

// Allocate a string block of sufficient size.

auto string = impure->vlu_string;

if (string && string->str_length < length)
{
delete string;
string = nullptr;
}

if (!string)
{
string = impure->vlu_string = FB_NEW_RPT(*tdbb->getDefaultPool(), length) VaryingString();
string->str_length = length;
}

impure->vlu_desc.dsc_address = string->str_data;
impure->allocateTextImpureDscAddress(*tdbb->getDefaultPool());
}

EVL_validate(tdbb, Item(Item::TYPE_CAST), itemInfo,
Expand Down Expand Up @@ -7139,29 +7122,7 @@ dsc* FieldNode::execute(thread_db* tdbb, Request* request) const
dsc desc = impure->vlu_desc;
impure->vlu_desc = format->fmt_desc[fieldId];

if (impure->vlu_desc.isText())
{
// Allocate a string block of sufficient size.
VaryingString* string = impure->vlu_string;

if (string && string->str_length < impure->vlu_desc.dsc_length)
{
delete string;
string = NULL;
}

if (!string)
{
string = impure->vlu_string = FB_NEW_RPT(*tdbb->getDefaultPool(),
impure->vlu_desc.dsc_length) VaryingString();
string->str_length = impure->vlu_desc.dsc_length;
}

impure->vlu_desc.dsc_address = string->str_data;
}
else
impure->vlu_desc.dsc_address = (UCHAR*) &impure->vlu_misc;

impure->makeImpureDscAddress(*tdbb->getDefaultPool());
MOV_move(tdbb, &desc, &impure->vlu_desc);
}

Expand Down Expand Up @@ -13687,30 +13648,7 @@ dsc* UdfCallNode::execute(thread_db* tdbb, Request* request) const
const Parameter* const returnParam = function->getOutputFields()[0];
value->vlu_desc = returnParam->prm_desc;

// If the return data type is any of the string types, allocate space to hold value.

if (value->vlu_desc.dsc_dtype <= dtype_varying)
{
const USHORT retLength = value->vlu_desc.dsc_length;
VaryingString* string = value->vlu_string;

if (string && string->str_length < retLength)
{
delete string;
string = NULL;
}

if (!string)
{
string = FB_NEW_RPT(*tdbb->getDefaultPool(), retLength) VaryingString;
string->str_length = retLength;
value->vlu_string = string;
}

value->vlu_desc.dsc_address = string->str_data;
}
else
value->vlu_desc.dsc_address = (UCHAR*) &value->vlu_misc;
value->makeImpureDscAddress(*tdbb->getDefaultPool());

if (!impureArea->temp)
{
Expand Down
14 changes: 1 addition & 13 deletions src/dsql/StmtNodes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2248,19 +2248,7 @@ const StmtNode* DeclareVariableNode::execute(thread_db* tdbb, Request* request,
variable->vlu_desc = varDesc;
variable->vlu_desc.clearFlags();

if (variable->vlu_desc.dsc_dtype <= dtype_varying)
{
if (!variable->vlu_string)
{
const USHORT len = variable->vlu_desc.dsc_length;
variable->vlu_string = FB_NEW_RPT(*tdbb->getDefaultPool(), len) VaryingString();
variable->vlu_string->str_length = len;
}

variable->vlu_desc.dsc_address = variable->vlu_string->str_data;
}
else
variable->vlu_desc.dsc_address = (UCHAR*) &variable->vlu_misc;
variable->makeImpureDscAddress(*tdbb->getDefaultPool());

request->req_operation = Request::req_return;
}
Expand Down
26 changes: 1 addition & 25 deletions src/jrd/SysFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5545,31 +5545,7 @@ dsc* evlMaxMinValue(thread_db* tdbb, const SysFunction* function, const NestValu

DataTypeUtil(tdbb).makeFromList(&impure->vlu_desc, function->name, argTypes.getCount(), argTypes.begin());

if (impure->vlu_desc.isText())
{
const USHORT length = impure->vlu_desc.dsc_length;

// Allocate a string block of sufficient size

auto string = impure->vlu_string;

if (string && string->str_length < length)
{
delete string;
string = nullptr;
}

if (!string)
{
string = impure->vlu_string = FB_NEW_RPT(*tdbb->getDefaultPool(), length) VaryingString();
string->str_length = length;
}

impure->vlu_desc.dsc_address = string->str_data;
}
else
impure->vlu_desc.dsc_address = (UCHAR*) &impure->vlu_misc;

impure->makeImpureDscAddress(*tdbb->getDefaultPool());
MOV_move(tdbb, result, &impure->vlu_desc);

if (impure->vlu_desc.dsc_dtype == dtype_text)
Expand Down
18 changes: 3 additions & 15 deletions src/jrd/evl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -573,22 +573,10 @@ void EVL_make_value(thread_db* tdbb, const dsc* desc, impure_value* value, Memor

// Allocate a string block of sufficient size.

VaryingString* string = value->vlu_string;
if (!pool)
pool = tdbb->getDefaultPool();

if (string && string->str_length < length)
{
delete string;
string = NULL;
}

if (!string)
{
if (!pool)
pool = tdbb->getDefaultPool();

string = value->vlu_string = FB_NEW_RPT(*pool, length) VaryingString();
string->str_length = length;
}
VaryingString* string = value->getAllocateString(from.dsc_length, *pool);
Copy link
Member

@dyemanov dyemanov Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be value->getAllocateString(length, *pool) ?


value->vlu_desc.dsc_length = length;
UCHAR* target = string->str_data;
Expand Down
41 changes: 41 additions & 0 deletions src/jrd/val.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,13 @@ struct impure_value
void make_double(const double val);
void make_decimal128(const Firebird::Decimal128 val);
void make_decimal_fixed(const Firebird::Int128 val, const signed char scale);

template<class T>
VaryingString* getAllocateString(T requeuedLength, MemoryPool& pool) = delete; // Prevent dangerous length shrink
VaryingString* getAllocateString(USHORT requeuedLength, MemoryPool& pool);
Copy link
Member

@dyemanov dyemanov Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call it just getString() for simplicity but I'm not going to insist.

Copy link
Member

@asfernandes asfernandes Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAllocateString is not a name that sounds good.

Copy link
Member

@asfernandes asfernandes Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually pool comes first in parameter list.

Copy link
Contributor Author

@Noremos Noremos Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the name to getString with the pool as the first parameter


void makeImpureDscAddress(MemoryPool& pool);
void allocateTextImpureDscAddress(MemoryPool& pool);
Copy link
Member

@dyemanov dyemanov Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're already inside the impure class, so please simplify the names, e.g. makeDscAddress() / setupDynamicDscAddress().

Copy link
Member

@asfernandes asfernandes Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These names are weird and maybe due to it, the methods too.
With current names, I prefer the original code.

Copy link
Contributor Author

@Noremos Noremos Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Names are my weak point. I tried others in the last commit, but it would be nice to get your suggestions.

};

// Do not use these methods where dsc_sub_type is not explicitly set to zero.
Expand Down Expand Up @@ -221,6 +228,40 @@ inline void impure_value::make_decimal_fixed(const Firebird::Int128 val, const s
this->vlu_desc.dsc_address = reinterpret_cast<UCHAR*>(&this->vlu_misc.vlu_int128);
}

inline VaryingString* impure_value::getAllocateString(USHORT requeuedLength, MemoryPool& pool)
Copy link
Member

@dyemanov dyemanov Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest newLength or just length instead of requeuedLength.

Copy link
Contributor Author

@Noremos Noremos Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

{
if (vlu_string && vlu_string->str_length < requeuedLength)
{
delete vlu_string;
vlu_string = nullptr;
}

if (!vlu_string)
{
vlu_string = FB_NEW_RPT(pool, requeuedLength) VaryingString();
vlu_string->str_length = requeuedLength;
}

return vlu_string;
}

inline void impure_value::makeImpureDscAddress(MemoryPool& pool)
{
if (vlu_desc.hasDynamicLength())
{
// If the data type is any of the string types, allocate space to hold value.
allocateTextImpureDscAddress(pool);
}
else
vlu_desc.dsc_address = (UCHAR*) &vlu_misc;
}

inline void impure_value::allocateTextImpureDscAddress(MemoryPool& pool)
{
vlu_desc.dsc_address = getAllocateString(vlu_desc.dsc_length, pool)->str_data;
}


struct impure_value_ex : public impure_value
{
SINT64 vlux_count;
Expand Down
Loading